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. 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. 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). 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. 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. > 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. > 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. > 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. Have a nice day :) Thomas
