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

[Rocky Bernstein]

On Thu, Jun 9, 2016 at 5:15 AM, Thomas Schmitt <address@hidden> wrote:

> Hi,
>
> Rocky Bernstein wrote:
> > Leave mmc_read_subchannel() as is.
> > Again it does a well-defined MMC function AND JUST THAT.
>
> But in the stack of functions to retrieve MCN and ISRC it is the
> highest one which knows the SCSI command.
>
> Handling of the sense data reply is part of SCSI command transaction.
> libcdio relies on the fact that ioctl(CDROM_SEND_PACKET) contains
> an own interpreter for sense data which may throw a UNIX error.
> ioctl(SG_IO) or the transport mechanisms of non-Linux systems do not
> indicate drive protests by bad return values and errno.
>

Again that's a bug in the gnu_linux driver. It should use ioctl(SG_IO) when
it understands
how to interpret sense data better.


>
> Nevertheless it is beneficial to do the handling where the best
> knowledge about the intention of the SCSI command is present.
> I would put it into mmc_get_mcn_isrc_private() which would not have
> to interpret the SCSI command to know what it is supposed to do.
>

That would be good. mc_get_mcn_isrc_private() is an internal private
routine in mmc.c. Since that routine is not exposed publically, feel free
to change it in mmc.c. I note that it already issues a couple of
mmc_read_subchannel() commands.


> Therefore my two other alternative proposals to not only record
> the sense data reply of the last SCSI command but also the command
> itself (i.e. mmc_cdb_t).
>

Let's not go this direction just yet.


>
> This idea raises the question what instance shall record mmc_cdb_t.
> My alternatives are a bottleneck macro or putting the plight onto
> the various implementations of p_cdio->op.run_mmc_cmd().
> Those implementations already record the returned sense data.
>
>
> > This new thing that has the parsed sense data and better error reporting,
> > this is a mmc_hl thing. Especially if it issues more than one MMC
> command.
>
> I don't think so. The sense data handling is bound to single SCSI
> commands. So mmc_hl is not the layer where this should be done.
>

I just rechecked the code and things can be added to mmc.c as well as
mmc_hl_cmd.c . But routines with MMC command names in mmc_ll_cmd.c, like
mmc_mode_sense_10(), mmc_get_configuration() or or mmc_read_subchannel()
among others, should issue at most one RUN_SCSI_CMD.


> The current situation is that Linux ioctl(CDROM_SEND_PACKET) does
> the job of sense data interpretation. Surely at least two levels of
> abstraction too deep. Neither the ioctl nor p_cdio->op.run_mmc_cmd()
> really know what an SCSI command shall do. So they cannot really
> judge the impact of problems on the intended side effect.
>

Again using that ioctl on GNU/Linux is a bug; it wasn't done intentionally
with the knowledge that the ioctl does sense interpretation or that this is
something that we should be aware of and should be a concern. Now that I've
been made aware of that, it should be addressed.


>
> > I feel like I've repeated this a couple of times:
>
> I cannot fit your wish to interpret sense data in mmc_hl with what
> i perceive as the model of MMC command execution in libcdio.
>
> Of course you are free to define an architecture to represent MMC.
> But this architecture has to match the way how SCSI transactions
> are used to perform the tasks of libcdio.
>

The way libcdio implements the operations in the drivers can be changed
transparently. That is largely true for routines in mmc.c or mmc_hl.c.
However routines in mmc_ll_cmds.c are also exposed to users and therefore
need to implement MMC commands one to one, and compatibility needs to be
maintained.

> I am not sure what you mean by:
> > > This prevents my plan to let error indicating sense data act like
> > > missing MCVAL/TCVAL bit. These bits are known only in
> > > mmc_get_mcn_isrc_private().
>
> mmc_get_mcn_isrc_private() knows that it shall retrieve MCN or ISRC.
> Both have the Valid bit at the same position in the reply data.
>
> The function  mmc_read_subchannel()  implements MMC command
> READ SUB-CHANNEL, which does not only MCN or ISRC but also can retrieve
> other info, which has no such Valid bit.
>
> So if i want to treat refusal by sense data the same as refusal by
> unset Valid bit, then i have to do this in  mmc_get_mcn_isrc_private().
> In mmc_read_subchannel() i can only indicate a transport failure.
>
>
> > >   ++ WARN: READ_SUBCHANNEL (CDB: 42 00 40 03 00 00 04 00 04 00 )
> > >   ++ WARN: [5 24 00] : Illegal Request, Invalid field in cdb
>
> > Looks great!
>
> The first of these lines causes the need for knowing the mmc_cdb_t.
>
> The tight association of both lines causes the need to evaluate and
> report sense data after each single SCSI command and not after each
> mmc_hl gesture.
>
> Leaving out any of these informations (or disassociating them) would
> hamper our ability to diagnose problem reports.
>

Addressed above.


>
>
> > Did you contact address@hidden ?
>
> I have now asked for a snail mail address where to send my personal info
> for getting the form. The initial curiosity of FSF in
>
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
> exceeds my willingness to tell by e-mail.
>
>
Sure. I think I sent them information by postal mail so that's definitely a
possibility. Yes, it's a bit of a chore, but a necessary one.
Thanks.



>
> Have a nice day :)
>
> Thomas
>
>
>