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 <[email protected]> wrote: > > > 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 >> >> >> >
