Details

Reviewers

ken
scottl
mav

Group Reviewers

cam

Commits

rGda73926566da: libcam: Define depop structures and introduce scsi_wrap

Summary

Define structures related to the depop set of commands (GET PHYSICAL ELEMENT
STATUS, REMOVE ELEMENT AND TRUNCATE, and RESTORE ELEMENT AND REBUILD) as
well as the CDB construction routines.

Also create scsi_wrap.c. This will have convenience routines that will do all
the elements of allocating the ccb, generating the CDB, sending the command
(looping as necessary for cases where data is returned, but it's size isn't
known up front), etc. As this functionality is fleshed out, calling many
camcontrol commands programatically gets much easier.

Test Plan

camcontrol depop uses these functions successfully today.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 37512
Build 34401: arc lint + arc unit

Event Timeline

imp created this revision.Mar 2 2021, 7:41 PM

Herald added a reviewer: cam. · View Herald TranscriptMar 2 2021, 7:41 PM

imp requested review of this revision.Mar 2 2021, 7:41 PM

Harbormaster completed remote builds in B37510: Diff 84995.Mar 2 2021, 7:41 PM

Spelling fixes

Harbormaster completed remote builds in B37512: Diff 84997.Mar 2 2021, 7:44 PM

imp edited the test plan for this revision. (Show Details)Mar 2 2021, 7:45 PM

imp added reviewers: ken, scottl, mav.

The CDBs and filling functions look good. We should figure out what our user interface idiom is going to be for the scsi_wrap_* functions. Do we assume we have stdout/stderr, or do we provide those strings for the user to print in a way that works for the calling application?

lib/libcam/scsi_wrap.c
94	So, what do we want our interface to be, or our "contract", when we're wrapping commands in libcam? Do we want to print to stdout/stderr by default, or provide a mechanism to get those strings back to the caller so that they can send them out via their logging interface or user interface? I'm just thinking here about some library (really Ruby Gem) code I've written for Spectra that does some things that are similar to camcontrol, but the errors are handled in a way that is more friendly to the calling Ruby management application.

In D29017#649782, @ken wrote:

The CDBs and filling functions look good. We should figure out what our user interface idiom is going to be for the scsi_wrap_* functions. Do we assume we have stdout/stderr, or do we provide those strings for the user to print in a way that works for the calling application?

I was thinking that the higher level programs should report the error, but I haven't figured out the proper way to do this yet. I was leaning towards the latter, but hadn't come up with a good way to do that. It just occurred to me, though, having a buffer in 'cam_device' would be the right way to handle the errors? The current cam_print_error interface, though, requires a ccb. We could use cam_error_string instead, I think and just buffer that and have the user print that if they want to use this convenience interface?

mav added inline comments.Mar 3 2021, 9:41 PM

lib/libcam/scsi_wrap.c
69	What number of 32-byte descriptors do you expect there be? If it is about heads, I'd guess about dozen. Wouldn't it be better to guess some size first to save one request in most cases?
77	Many tools were switched to calloc() to not call bzero explicitly.

imp added inline comments.Mar 4 2021, 12:09 AM

lib/libcam/scsi_wrap.c
69	chuck had the same feedback. I just did 4k since that's big enough to return lots of elements, and small enough to not matter.
77	Sure. That's not a bad idea.
94	I'm thinking of adding an error sbuf to cam_device that accumulates errors that the calling process can use to report errors and then cleanup...

Use calloc, per mav. Use 4k allocation per chs and mav.

Harbormaster completed remote builds in B37559: Diff 85097.Mar 4 2021, 12:11 AM

One minor comment.

lib/libcam/scsi_wrap.c
94	So what about threaded processes with multiple threads accessing the same device? I guess we'll expect the caller to lock that as needed, just as they would have to if they were accessing any other fields in there. That's probably a reasonable way to handle the static cam_errbuf issue with multiple threads/devices potentially putting error messages in it.

lib/libcam/scsi_wrap.c
94	I think that's a sensible strategy.

update based on review

Harbormaster completed remote builds in B40761: Diff 93005.Jul 30 2021, 10:21 PM

imp marked 2 inline comments as done.Sep 17 2021, 10:32 PM

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2021, 10:40 PM

Closed by commit rGda73926566da: libcam: Define depop structures and introduce scsi_wrap (authored by imp). · Explain Why

This revision was automatically updated to reflect the committed changes.

imp added a commit: rGda73926566da: libcam: Define depop structures and introduce scsi_wrap.

libcam: Define depop structures and introduce scsi_wrap
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 84997

lib/libcam/Makefile

lib/libcam/scsi_wrap.h

lib/libcam/scsi_wrap.c

sys/cam/scsi/scsi_all.h

sys/cam/scsi/scsi_all.c

libcam: Define depop structures and introduce scsi_wrapClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 84997

lib/libcam/Makefile

lib/libcam/scsi_wrap.h

lib/libcam/scsi_wrap.c

sys/cam/scsi/scsi_all.h

sys/cam/scsi/scsi_all.c

libcam: Define depop structures and introduce scsi_wrap
ClosedPublic
Actions

Revision Contents
Changeset List