[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Rocky Bernstein
Subject: Re: [Libcdio-devel] Read Sub-channel changes
Date: Thu, 9 Jun 2016 08:00:08 -0400

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

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

> Have a nice day :)
> Thomas

reply via email to

[Prev in Thread] Current Thread [Next in Thread]