Re: [Libcdio-devel] Read Sub-channel changes

[Thomas Schmitt]

Hi,

> Again, we should not contemplate changing something that exists

I am well aware of the goal to keep API and ABI stable.
Further i do not want to break things without a good excuse prepared.


> It would be fine to create a function that runs a mmc command and then
> issues a sense command.  Since it issues the mmc command it knows what
> command it ran.

I understand you are in favor of this alternative:

> > - Move sense code evaluation and reporting to functions which know
> >   the mmc_cdb_t. This knowledge is currently quite low-level and very
> >   volatile.

So i will move the call of mmc_evaluate_last_sense() from
mmc_get_mcn_isrc_private() to mmc_read_subchannel().

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().
In mmc_read_subchannel() i can only return something like DRIVER_OP_ERROR.

Is DRIVER_OP_ERROR the appropriate indicator for invalid reply data ?

A report (deliberately provoked by wrong track number 4) now looks like this

  ++ WARN: READ_SUBCHANNEL (CDB: 42 00 40 03 00 00 04 00 04 00 )
  ++ WARN: [5 24 00] : Illegal Request, Invalid field in cdb

(Shall i downgrade it to INFO ?
 Ignorable problems will be reported by INFO.)

This is the call in mmc_read_subchannel which causes the warnings:

  if(mmc_evaluate_last_sense(p_cdio, &cdb, CDIO_MMC_EVAL_SENSE_WARN)
      > CDIO_MMC_EVAL_SENSE_IGN)
    i_status = DRIVER_OP_ERROR;

It should be executed in low level MMC functions immediately after

  i_status = MMC_RUN_CMD(SCSI_MMC_DATA_READ, i_timeout_ms);

This is not ideal because DRIVER_OP_ERROR could override other errors.
But if sense data was evaluated only if (i_status == DRIVER_OP_SUCCESS)
then ioctl(CDROM_SEND_PACKET) would obscure the detailed error message
by its errno == EIO.

I guess this is the appropriate companion macro for MMC_RUN_CMD

  #define MMC_EVAL_SENSE(i_status, report_mode) \
     if(mmc_evaluate_last_sense(p_cdio, &cdb, report_mode) \
        > CDIO_MMC_EVAL_SENSE_IGN) \
       i_status = DRIVER_OP_ERROR;

which reduces the footprint of above evaluation to

  MMC_EVAL_SENSE(i_status, CDIO_MMC_EVAL_SENSE_WARN)

(Shall i leave out the ';' in the macro so that its invocation needs one ?)

---------------------------------------------------------------------------

If this mechanism gets accepted then one would next have to sprinkle its
use all over those parts of lib/driver/mmc/ which call op.run_mmc_cmd().

Of course one may only change those code parts which one is able to test.


Have a nice day :)

Thomas