On Thu, Jun 9, 2016 at 5:15 AM, Thomas Schmitt <[email protected]> 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 [email protected] ? > > 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 > > >
