Re: [Libcdio-devel] Read Sub-channel changes

[Rocky Bernstein]

run_mmc_cmd in the cdio_func_t should be left alone for backwards
compatibility and because it implements a well-defined concept defined (if
incomplete by itself) function of the MMC standard.

Again, we should not contemplate changing something that exists and that
people might be using for something proposed however better, but might need
adjustment and adoptance time.

It would be fine to create a function that runs a mmc command and then
issues a sense command.  Since it issues the mmc command it knows what
command it ran.

Things that bundle MMC commands are put in mmc_hl.c .

If this is not clear, look at mmc_eject_media which allows media removal
before issuing, mmc_start_stop_unit. Or mmc_mode_sense which can issue two
mode sense commands.  If there are reasonable function that only need to
issue mode_sense commands under certain conditions, then again that goes
into mmc_hl.c.



On Wed, Jun 8, 2016 at 11:34 AM, Thomas Schmitt <address@hidden> wrote:

> Hi,
>
> when testing the sense code evaluator and its messages i got to a
> problem of layering and the encapsulated knowledge about the performed
> SCSI command (mmc_cdb_t).
> I see the following alternatives:
>
> - Move sense code evaluation and reporting to functions which know
>   the mmc_cdb_t. This knowledge is currently quite low-level and very
>   volatile.
>
> - Bottleneck all calls to p_cdio->op.run_mmc_cmd() through MMC_RUN_CMD()
>   which would then record the mmc_cdb_t before the command gets
>   executed.
>
> - Put responsibility for recording mmc_cdb_t on each implementation
>   of p_cdio->op.run_mmc_cmd(). This is already done for sense data,
>   because they have to be copied from driver specific memory to
>   generic_img_private_t.
>
> --------------------------------------------------------------------
> Reasoning:
>
> The sense code messages are incomplete if they are not accompanied
> by info about the SCSI command which got them as reply.
> But the functions which could best judge the impact of sense codes
> are too high in the layer stack to know the SCSI command CDBs.
>
> So additionally to the last received sense data, generic_img_private_t
> should get a buffer to record the SCSI command which caused the buffered
> sense data.
>
>     typedef struct {
>       ...
>       mmc_cdb_t scsi_mmc_cmd;
>       int scsi_mmc_cmd_len;
>       ...
>     } generic_img_private_t;
>
> These new struct members could be filled by  MMC_RUN_CMD()  which knows
> them as cdb and mmc_get_cmd_len(cdb.field[0]).
>
> Regrettably not all SCSI command transactions are performed via
> MMC_RUN_CMD(). There are direct uses of p_cdio->op.run_mmc_cmd()
> and even function pointers handed down to other functions.
>
> How many implementations of SCSI command transport do we have ?
>
> --------------------------------------------------------------------
>
> The problem is partly hidden by run_mmc_cmd_linux(), because with severe
> sense codes, the ioctl throws an error and run_mmc_cmd_linux() reports
>
>    INFO: ioctl CDROM_SEND_PACKET for command READ_SUBCHANNEL (0x42) failed:
>         Input/output error
>
> before mmc_get_mcn_isrc_private() can call mmc_evaluate_last_sense()
> which then says
>
>    INFO: [5 24 00] : Illegal Request, Invalid field in cdb
>
> I am quite sure that ioctl(SG_IO) would not throw a Unix error in
> the same situation. We cannot rely on the SCSI command name to be yelled
> by the SCSI transport layer when interesting sense data arrive.
>
> ioctl(CDROM_SEND_PACKET) will not throw an error command name if a harmless
> sense code is encountered. But we still might want to report it. Possibly
> via cdio_error().
>
> --------------------------------------------------------------------
>
> Have a nice day :)
>
> Thomas
>
>
>