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

[Rocky Bernstein]

On Wed, Jun 8, 2016 at 4:47 PM, Thomas Schmitt <address@hidden> wrote:

> 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.
>

Great!


>
>
> > 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 is not quite what I had in mind. Leave mmc_read_subchannel() as is.
Again it does a well-defined MMC function AND JUST THAT.

Add something different. It is always possible in future releases to
deprecate mmc_read_subchannel() and then remove it.

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 feel like I've repeated this a couple of times: mmc_ll.c has one MMC
command and no more. mmc_hl.c can have one or more MMC commands.  LL means
= MMC. HL a more useful way of using MMC.  HL should have access to as much
low-level bits and structures that LL has. So 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().
>


> In mmc_read_subchannel() i can only return something like DRIVER_OP_ERROR.
>

This new thing that gets written can have a parameter return value.


>
> Is DRIVER_OP_ERROR the appropriate indicator for invalid reply data ?
>

Of the enumerations that are currently listed, that looks fine to me. But
if you feel strongly that there are some more specific error cases that
should be indicated, we can add to the list of enumerations.


>
> 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
>

Looks great!


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

Sure, use your own judgement. If you think so then do it that way. If this
turns out to be a problem, we can change it later.


> 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 ?)
>

Sure. This all sounds great.



>
> ---------------------------------------------------------------------------
>
> 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().
>

Yep. But that is something libcdio develops have to do or may opt to do,
since everything is compatible.

And users may opt into the better way of doing things as well, but are not
forced to.


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

Yep. Thanks for working on this. Did you contact address@hidden ?


>
>
> Have a nice day :)
>
> Thomas
>
>
>