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

[Rocky Bernstein]

See I omitted one point.

mmc_get_mcn_isrc_private() I guess needs something better than the existing
mmc_read_subchannel(). No problem. Create something new that has the right
interface, i guess it would pass back a cdb. And call that in
mmc_get_mcn_isrc_private.  This new routine can go in mmc_ll_cmds.c

On Thu, Jun 9, 2016 at 8:00 AM, Rocky Bernstein <address@hidden> wrote:

>
>
> 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
>>
>>
>>
>