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

[Thomas Schmitt]

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