Thomas: I think you are correct about the routines which go into mmc_hl_cmds.c and mmc_ll_cmds.c.
I'm not sure what: (Especially interesting is if CDIO_MMC_SET_READ_LENGTH8() gets value 8 > regardless of *num_data which acts as parameter i_buf of mmc_run_cmd().) > means. You think there is a bugi n that macro definition? The definition seems fine: #define CDIO_MMC_SET_READ_LENGTH8(cdb, len) \ cdb[8] = (len ) & 0xff This sets the "read length" field to a length value that fits into a single byte. This looks fine. Again, what I'd suggest is to create a branch off of master and commit your changes. As before, if git is not your thing, then attach a diff as a patch. I find working off of diffs in the mailing list archive a little bit wonky. Thanks. On Sun, Apr 3, 2016 at 2:16 PM, Thomas Schmitt <[email protected]> wrote: > Hi, > > i tested my favorite MNC / ISRC retriever. It needed some corrections. > (Especially interesting is if CDIO_MMC_SET_READ_LENGTH8() gets value 8 > regardless of *num_data which acts as parameter i_buf of mmc_run_cmd().) > > This is now tested against nightcats_ii.right (which seems to have been > generated without cd-info option --no-header, i fear). > > > int > mmc_read_sub_channel_transp ( void *p_env, > const mmc_run_cmd_fn_t run_mmc_cmd, > track_t i_track, > unsigned char sub_chan_param, > unsigned int *num_data, > char *buf > ) > { > mmc_cdb_t cdb = {{0, }}; > int i_status; > > if ( ! p_env || ! run_mmc_cmd ) > return -1; > if (*num_data < 4) > return -1; > > CDIO_MMC_SET_COMMAND(cdb.field, CDIO_MMC_GPCMD_READ_SUBCHANNEL); > CDIO_MMC_SET_READ_LENGTH8(cdb.field, *num_data); > > cdb.field[1] = 0x0; > cdb.field[2] = 0x40; > cdb.field[3] = sub_chan_param; > cdb.field[6] = i_track; > > memset(buf, 0, *num_data); > > i_status = run_mmc_cmd(p_env, mmc_timeout_ms, > mmc_get_cmd_len(cdb.field[0]), > &cdb, SCSI_MMC_DATA_READ, > *num_data, buf); > if(i_status == 0) { > *num_data = (((unsigned int) buf[2]) << 8) + (unsigned int) buf[3] + 4; > return 0; > } > return -1; > } > > char * > mmc_read_mcn_isrc ( void *p_env, > const mmc_run_cmd_fn_t run_mmc_cmd, > track_t i_track, > unsigned char sub_chan_param, > int length > ) > { > char buf[24]; /* Maximum reply size as of MMC-4 */ > unsigned int num_data; > > if (length > sizeof(buf) - 9) > return NULL; /* Too many reply bytes requested */ > > /* inquire number of available reply bytes */ > num_data = 4; > if (mmc_read_sub_channel_transp (p_env, run_mmc_cmd, i_track, > sub_chan_param, &num_data, buf) == -1) > return NULL; > if (num_data < 9 + length) > return NULL; /* Not enough data replied */ > if (num_data > sizeof(buf)) > num_data = sizeof(buf); > > /* now fetch data */ > if (mmc_read_sub_channel_transp (p_env, run_mmc_cmd, i_track, > sub_chan_param, &num_data, buf) == -1) > return NULL; /* SCSI command failed */ > > /* pick MCN or ISRC */ > if (num_data < 9 + length) > return NULL; /* Not enough data replied */ > if ( ! (buf[8] & 0x80) ) > return NULL; /* MCN/ISRC not valid */ > return strndup(&buf[9], length); > } > > > I added the prototypes of the functions to > ./lib/driver/mmc/mmc_private.h > and removed the prototypes of the old two functions. > > The callers of the old functions were changed to use mmc_read_mcn_isrc(). > > --------------------------------------------------------------------- > > Rocky Bernstein wrote: > > add a routine to try the 2-step approach if that's the way you think > best. > > Well, above is my tested functional sketch. > > > As for overall code design, let me review how I've broken out the code. > The > > file mmc_hl.c was intended for this kinds of approaches where we try a > > couple of ways to do get a logical function done using more lower-level > > primitives in mmc_ll.c and mmc.c. > > I assume you mean ./lib/driver/mmc/mmc_[hl]l_cmds.c . > > So > mmc_read_sub_channel_transp() > would go to mmc_ll_cmds.c as > mmc_read_sub_channel() > and would need some of its entrails to be translated to macros like > MMC_CMD_SETUP() , MMC_RUN_CMD() > ? > > The function > char * > mmc_read_mcn_isrc() > would go to mmc_hl_cmds.c as > driver_return_code_t > mmc_get_mcn_isrc ( ... parameters or mmc_read_mcn_isrc() ... , > char *result_text); > ? > > --------------------------------------------------------------------- > > Rocky: I think this is the point where you are more qualified than me. > If you want to get something done right ... > > The changeset motivation text could be: > > "Obtain by SCSI command READ SUB-CHANNEL only as many bytes > as available and as are necessary for MCN or ISRC. Only > return the result text to higher levels if the reply is marked > as valid by the MCVAL or TCVAL bit." > > Regrettably we cannot claim in advance that this would solve > the ISRC misattribution in Feodora bug report 1321677. > > > Have a nice day :) > > Thomas > > >
