[sane-devel] RFC: proposal for an improved sanei_scsi library

[abel deuring]

Henning Meier-Geinitz wrote:
> Hi,
> 
> On Sat, Dec 07, 2002 at 11:29:20PM +0100, abel deuring wrote:
> 
>>Some time ago, I made some remarks on this list about things I don't 
>>like in the sanei_scsi interface: 
>>http://www.mostang.com/pipermail/sane-devel/2002-January/001463.html
>>Here is a proposal for a modified interface. The main points are:
> 
> 
> Looks basically ok for me but I don't know much about SCSI :-)
> 
> I would just switch to the new API in SANE2 and use e.g.
> sanei_scsi_open (not sanei2_). All backends must be rewritten anyway.
> So you avoid confusion if it's necessary to add a sanei_scsi_open2 in
> future.

Well, there is indeed not urgent need to change the interface for Sane1. 
But there were a few situations where I wished I had a somewhat cleaner 
interface at hand for Sane 1 ;) And it would not be difficult to write a 
compatibility layer for the current interface. But if Sane2 development 
is taking off soon, work on Sane1 would of course be useless.

> 
> 
>>Instead, the sense buffer and the device status byte are passed to the 
>>caller by sanei_scsi_req_wait / sanei_scsi_cmd.
> 
> 
> What is the "device status byte"? Is this defined in the SCSI spec?
> With the old interface, there was no way to get it?

Right, the old interface simply hided the SCSI status byte.

A SCSI device returns a status byte for every command, giving coarse 
information about the result of the command (GOOD/CHECK 
CONDITION/CONDITION MET/BUSY etc). A detailed description can be found 
in the SCSI2 draft, ftp://ftp.t10.org/t10/drafts/s2/s2-r10l.pdf , 
section 7.3. The SCSI reference manuals for the scanners should describe 
it too. While it is mostly the job of the SCSI subytsem of the OS and 
the sanei_scsi layer to deal with the device status, it may contain some 
useful stuff for a backemd. For example. a device can return the status 
RESERVATION CONFLICT, if a host tries to access the device, while 
another host had the device "locked" with the RESERVE UNIT command.

> 
> 
>>- sanei2_scsi_req_flush_all now only flushes the command for a single 
>>device file. This is perhaps not very important, since the frontends I 
>>know do not run more than one scan at a time, and it is not very likely 
>>that we'll ever have a frontend that tries to control 10 scanners 
>>simultaneously -- but a clean implementation would allow this ;) (And we 
>>are not that far away from such an implementation.)
> 
> 
> The SANE sttandard requires, that backends can open several devices
> simultaneously. So I think it is important. And even if there is no
> frontend yet that supports opening multiple devices it wouldn't be
> that unusual to just keep the device selection window open to select
> more scanners.

Seems that I should re-read the Sane 2 drafts ;)

> 
> By the way: simultaneous scanning with some SCSI and some USB scanners
> and multiple copies of XSane does work :-)
> 
> 
>>/** Find SCSI devices.
>> *
>> * Find each SCSI device that matches the pattern specified by the
>> * arguments.  String arguments can be "omitted" by passing NULL,
>> * integer arguments can be "omitted" by passing -1.
>> *
>> *   Example: vendor="HP" model=NULL, type=NULL, bus=3, id=-1, lun=-1 would
>> *          attach all HP devices on SCSI bus 3.
>> *
>> * @param vendor
>> * @param model
>> * @param type
>> * @param bus
>> * @param channel
>> * @param id
>> * @param lun
>> * @param attach callback invoked once for each device
>> * @param dev real devicename (passed to attach callback)
>> *
>> */ 
> 
> 
> While you are at it you could add descriptions for params vendor to
> lun :-)

Uuuuhhh, yes ;) This is one of the points where it is obvious that I 
simply took the old sanei_scsi.h file and edited a few lines ;)

> 
> 
>>/** Open a SCSI device
>> *
>> * Opens a SCSI device by its device filename and returns a file descriptor.
>> * The function tries to allocate a buffer of the size given by *buffersize.
>> * If sanei2_scsi_open returns successfully, *buffersize contains the
>> * available buffer size. This value may be both smaller or larger than the
>> * value requested by the backend; it can even be zero. The backend must not
>> * try to issue SCSI command with data blocks larger than given by the
>> * value of *buffersize. The backend must decide, if it got enough buffer
>> * memory to work.
> 
> 
> What about timeout? Can it be changed by sanei_scsi, too? If yes, I
> would add a sentence about this. Otherwise it could be just int
> timeout instead of int * timeout.

Well, using a pointer argument was meant for the situation that as OS 
might not allow to easily change the timeout value (I'm not aware of any 
such OS, but you never know). In this case, sanei_scsi_open could simply 
set the default value.

> 
> 
>> * @param devicename name of the devicefile, e.g. "/dev/sg0"
>> * @param fd file descriptor
>> * @param is the timeout in seconds for SCSI commands
> 
> 
> @param timeout is the timeout in seconds for SCSI commands
> 
> 
>> * @param sense_arg pointer to data for the sense handler
> 
> 
> Which sense handler?

sense_arg is of course a relict from the old interface...

> 
> 
>> * @sa sanei2_scsi_open(), HAVE_SANEI_SCSI_OPEN_EXTENDED
> 
> 
> @sa is "see also", you can remove this line in this case.

ok.

> 
> 
>>/** Enqueue SCSI command
>> *
>> * One or more scsi commands can be enqueued by calling sanei2_scsi_req_enter().
>> *
>> * NOTE: Some systems may not support multiple outstanding commands.  On such
>> * systems, sanei2_scsi_req_enter() may block.  In other words, it is not proper
>> * to assume that enter() is a non-blocking routine.
>> *
>> * @param fd file descriptor
>> * @param cmd pointer to SCSI command
>> * @param cmd_size size of the command
>> * @param buffer pointer to the buffer with data to be sent to / received from
>>          the scanner
> 
> 
>    *        the scanner
>    
> 
>> * @param buffer_size size of the data buffer
>> * @param direction direction of the data transfer. Allowed values:
>> *	  - SANE_SCSI_DXFER_NONE  no data transfer
>> *        - SANE_SCSI_DXFER_TO_DEVICE data is sent to the device
>> *	  - SANE_SCSI__DXFER_FROM_DEVICE data is received from the device
> 
> 
> Use SANEI_, as this is not a SANE API definition. The #defines for
> these constants should be in the header, too. In this case, there
> would be a nice cross-reference to their descriptions if you use the
> doxygen HTML pages.
> 
> Minor formatting issue:
> The formatting seems to be wrong here. Looks like the "-" don't work
> in @param sections. When you explain the SANEI_SCSI_ stuff at the
> point where they are #defined, you could just list them here without
> the "-".
> 
> 
>> * @sa sanei2_scsi_req_enter()
> 
> 
> Remove.
> 
> 
>>/** Wait for SCSI command
>> *
>> * Wait for the completion of the SCSI command with id ID.  
>> *
>> * @param id id used in sanei2_scsi_req_enter()
>> * @param sb pointer to a SANE_Byte array, where the SCSI sense data may be
>> *           stored. The caller must allocate the necessary memory.
>> *	     The parameter may be NULL.
> 
> 
> Why is SANE_Byte used here while all the other types are non-SANE
> types? If you want to make sure that it's a byte (as in 8 bits)
> buffer, maybe it's better to use u_int8_t or something like that.
> SANE_Byte must be able to hold 8 bits, but it may be bigger than that.

Good question ;) Well, we could also simply use an int.

> 
> 
>> * @param scsi_status the status returned by the device. 
>> *           xxx what about the situation, that the command has not been
>> *           issued, or other situations, where no status information
>> *           is available? Can this situation be detected for all supported
>> *           OSes? If that is possible, we should add another parameter which
>> *	     signals "SCSI status available"
> 
> 
> Maybe use the return value to flag this. I.e. i/o error: scsi_status
> has some meaning, invalid arg: something different went wrong.

I don't like to put more semantics into the return status, But we could 
add another int* parameter which signals, if *scsi_status contains a 
valid value.

> 
> 
>>/** Send SCSI command
>> *
>> * This is a convenience function that is equivalent to a pair of
>> * sanei2_scsi_req_enter()/sanei2_scsi_req_wait() calls.
> 
> 
> [...]
> 
> 
>> *
>> * @return
>> * - SANE_STATUS_GOOD - on success
>> * - SANE_STATUS_IO_ERROR - if an error was received from the SCSI driver
>> * - SANE_STATUS_NO_MEM - if malloc failed (not enough memory)
>> * - SANE_STATUS_INVAL - if a locking or an unknown error occured
>> * - SANE_STATUS_DEVICE_BUSY  - the SCSI command could not be issued; try
>> *                       again later
>> * @sa sanei2_scsi_cmd2(), sanei2_scsi_req_enter(), sanei2_scsi_req_wait()
> 
> 
> Remove the first reference.
> 
> 
>> */
>>extern SANE_Status sanei2_scsi_cmd2 (int fd,
>>				   const void * cmd, size_t cmd_size,
>>				   const void * buffer, size_t buffer_size,
>>				   int direction, SANE_Byte *sb, size_t *sblen,
>>				   SANE_Byte *scsi_status);
> 
> 
> 
> why scsi_cmd2?

incomplete editing ;)