Page MenuHomeFreeBSD
Differential D29017

libcam: Define depop structures and introduce scsi_wrap
ClosedPublic
Actions

Authored by imp on Mar 2 2021, 7:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 30, 8:40 AM
Unknown Object (File)
Sat, Mar 28, 8:57 AM
Unknown Object (File)
Fri, Mar 27, 10:07 AM
Unknown Object (File)
Mon, Mar 23, 6:40 PM
Unknown Object (File)
Mon, Mar 23, 5:25 AM
Unknown Object (File)
Sun, Mar 22, 5:20 AM
Unknown Object (File)
Sun, Mar 22, 5:20 AM
Unknown Object (File)
Sat, Mar 21, 10:57 AM
View All Files
Subscribers
None

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
imp updated this revision to Diff 84997.Mar 2 2021, 7:44 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.
ken added a comment.Mar 2 2021, 8:52 PM

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.

imp added a comment.Mar 3 2021, 12:34 AM
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...

imp updated this revision to Diff 85097.Mar 4 2021, 12:11 AM

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
ken added a comment.Mar 4 2021, 5:02 PM

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.

imp marked an inline comment as done.Jul 12 2021, 11:10 PM
lib/libcam/scsi_wrap.c
94

I think that's a sensible strategy.

imp updated this revision to Diff 93005.Jul 30 2021, 10:21 PM

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.

Revision Contents
Changeset List

PathSize
lib/
libcam/
4 lines
34 lines
123 lines
sys/
cam/
scsi/
85 lines
82 lines

Diff 84997

View Options

lib/libcam/Makefile

Loading...
View Options

lib/libcam/scsi_wrap.h

Loading...
View Options

lib/libcam/scsi_wrap.c

Loading...
View Options

sys/cam/scsi/scsi_all.h

Loading...
View Options

sys/cam/scsi/scsi_all.c

Loading...