Re: [Libcdio-devel] Fedora bug report concerning CD-TEXT

[Rocky Bernstein]

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 <address@hidden> 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
>
>
>