Add scsi_cdb_sbuf()

[scottl]

Summary

Add sbuf-enabled handling of the CDB. Reimplement scsi_cdb_string() in terms
of scsi_cdb_sbuf() so that extra stack storage isn't needed.

[scottl]

Note that I intend to switch a few consumers of scsi_cdb_string(), notably scsi_command_string() to use this variant in order to streamline the use of sbufs and eliminate the need for stack storage.

[imp]

This looks good to me. One niggle that I don't understand...

sys/cam/scsi/scsi_all.c

3537–3538 ↗

(On Diff #15133)

How is this possible. Foo & 0x7 necessarily yields a number between 0 and 7. We have all cases between 0 and 7 already.

[scottl]

sys/cam/scsi/scsi_all.c

3537–3538 ↗

(On Diff #15133)

Just defensive programming.

[mav]

I have no objections if you have a good use for it.

I have some general bias against sbuf's in CAM, which in most part runs under mutexes and so prone to memory allocation failures. But since this is by far not the first case -- this is not a stopper.

[scottl]

I agree with the worry over the use of mutexes and allocation failures. I thing I considered was having scsi_cdb_string() take an option for whether to allocate the sbuf off the stack or dynamically. What do you think?

[mav]

I don't think options like that are useful. I doubt they are ever set consciously. More likely either set always or not set ever. Neither case is useful.

[ken]

Overall I think this is fine. See my comments on the printfs.

sys/cam/scsi/scsi_all.c

3471 ↗

(On Diff #15133)

Since these are generally library functions, doing a straight printf is a little iffy. But, that said, if we're going to print, we should prefix it with the function name so it is obvious where the message is coming from. i.e.:

printf("%s: len == 0\n", func);

3477 ↗

(On Diff #15133)

Same thing applies here.

[scottl]

sys/cam/scsi/scsi_all.c

3471 ↗

(On Diff #15133)

This might have been a debug printf that I forgot to remove, I'll take a closer look

[ken]

On second thought, sbuf_new() does a M_WAITOK malloc for the kernel version of it.

This could turn places that are okay now into locking problems.

The sbuf isn't very large, how about just allocating that on the stack?

[scottl]

Removed some extraneous debugging. Made scsi_cdb_string() use on-stack
storage for the sbuf in order to improve reliability.

[ken]

Looks good, thanks for making the change!