Re: [Libcdio-devel] mmc_get_disc_erasable proposition + questions

[Rocky Bernstein]

There are a number of different going on in what you write.  Let me try to
at least start to address some of them. So again I have to apologize if this
is long or if I have omitted something.

And as usual, I welcome thoughts and comments of others.

With regards to mmc_get_disc_info_data_if_null which is a wrapper for the
MMC's READ DISC INFO command, the wrapper aspect and allocation of a place
to put the data seems okay. I am not sure I like the idea though of a
routine which tests one of the parameters not NULL and returns
DRIVER_OP_SUCCESS if it is.

Instead, the information from READ DISC INFO  should be stored internally
inside a structure such as in the Cdio_t structure. And right now I am
contemplating adding a private mmc section of a Cdio_t and moving the sense
information in there.

Since mmc_get_disc_info_data_if_null is a wrapper for a MMC command, I think
the name should reflect that fact. For example, possibly a more appropriate
name is mmc_read_disc_info.

Note that there currently are other corresponding wrapper's to MMC commands:
READ CD, MODE SENSE 6, MODE SENSE 10, SET SPEED, START STOP (whose libcdio
mmc command name  adds "_media" to the end of the name -- I don't know if
this was a good idea or not. Thoughts?).

If there is a way to reduce the template aspect of this code more, such as
through additional functions or a C preprocessor macro, I think that would
be a good thing to do.
Over time, I imagine there will be many more of these boilerplate wrapper
routines.

So right now I'm contemplating on splitting out all of these purely MMC
wrapper routines a file separate from mmc.c, possibly called mmc_cmds.c.
Along with this, another C header would be created. So if your application
just could use wrapper libcdio mmc routines it might not need to know say
what how to set or retrieve a 24-bit CDB length field (CDIO_MMC_SET_LENGTH,
CDIO_MMC_GET_LENGTH) which currently is in mmc.h.

With regards to mmc_disc_erasable(), I am delighted to see it using the new
routine proposed (which might get modified as above).

With regards to mmc_get_disc_empty(), adding such a function is fine.
Probably we need to add something to mmc.h so
   (*mmc_disc_info_data)[2] & 0x03 == 0x00

is not as opaque.

I don't like the disc_information_t as a public structure. As a private
structure it might be okay. Instead I'd prefer access functions for parts
inside of it.

Finally, you are correct that cdio_is_discmode_dvd is missing that
CDIO_DIC_MODE_OTHER case. I've now added that into git as well as corrected
and updated the DVD Book values based on Thomas' observations and
suggestions.

Thanks for the suggestions and proposed code. In sum of course we're still
interested. I think a little more work is needed on some of them.


On Sat, Feb 6, 2010 at 5:21 AM, Frank Endres <address@hidden>wrote:

> Hi !
>
> I followed suggestions to avoid unneeded "fat" status-retrieving by MMC
> operation. Wwat do You think about it ?
> /**
>  Retrieves MMC DISC_INFO_DATA.
>
>  @param p_cdio the CD object to be acted upon.
>
>  @param i_status, on return will be set indicate whether the operation was
>  a success (DRIVER_OP_SUCCESS) or if not to some other value.
>
>  @param mmc_disc_info_data, if *mmc_disc_info_data is NULL, an MMC command
>  will be performed to retrieve the data, else, nothing will be done. Free
> when
>  no longer needed.
>  */
> void
> mmc_get_disc_info_data_if_null( const CdIo_t *p_cdio,
>                           driver_return_code_t *i_status,
>               uint8_t **mmc_disc_info_data) {
>    mmc_cdb_t cdb = {{0, }};
>
>    if (*mmc_disc_info_data == NULL) {
>    *mmc_disc_info_data = (uint8_t*) malloc (MMC_DISC_INFO_DATA_SIZE *
> sizeof (uint8_t));
>    memset (*mmc_disc_info_data, 0, MMC_DISC_INFO_DATA_SIZE);
>    CDIO_MMC_SET_COMMAND (cdb.field, CDIO_MMC_GPCMD_READ_DISC_INFO);
>    CDIO_MMC_SET_READ_LENGTH8 (cdb.field, MMC_DISC_INFO_DATA_SIZE);
>
>    *i_status = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                MMC_DISC_INFO_DATA_SIZE, *mmc_disc_info_data);
>    } else {
>    i_status = DRIVER_OP_SUCCESS;
>    }
> }
>
>
> /**
>  Detects if a disc (CD or DVD) is erasable or not.
>
>  @param p_cdio the CD object to be acted upon.
>
>  @param opt_i_status, if not NULL, on return will be set indicate whether
>  the operation was a success (DRIVER_OP_SUCCESS) or if not to some
>  other value.
>
>  @param mmc_disc_info_data, if *mmc_disc_info_data is NULL, an MMC command
>  will be performed to retrieve the data, else, the available data will be
>  used. Free when no longer needed.
>
>  @return true if the disc is detected as erasable (rewritable), false
>  otherwise.
>  */
> bool
>    mmc_get_disc_erasable( const CdIo_t *p_cdio,
>                           driver_return_code_t *opt_i_status,
>               uint8_t **mmc_disc_info_data) {
>    driver_return_code_t i_status;
>
>    mmc_get_disc_info_data_if_null (p_cdio, &i_status, mmc_disc_info_data);
>    if (opt_i_status != NULL) *opt_i_status = i_status;
>
>    return (DRIVER_OP_SUCCESS == i_status) ?
>        (((*mmc_disc_info_data)[2] & 0x10) ? true : false)
>        : false;
> }
>
>
> /**
>  Detects if a disc (CD or DVD) is empy or not.
>
>  @param p_cdio the CD object to be acted upon.
>
>  @param opt_i_status, if not NULL, on return will be set indicate whether
>  the operation was a success (DRIVER_OP_SUCCESS) or if not to some
>  other value.
>
>  @param mmc_disc_info_data, if *mmc_disc_info_data is NULL, an MMC command
>  will be performed to retrieve the data, else, the available data will be
>  used. Free when no longer needed.
>
>  @return true if the disc is detected as empty, false otherwise.
>  */
> bool
>    mmc_get_disc_empty( const CdIo_t *p_cdio,
>                           driver_return_code_t *opt_i_status,
>              uint8_t **mmc_disc_info_data) {
>    driver_return_code_t i_status;
>
>    mmc_get_disc_info_data_if_null (p_cdio, &i_status, mmc_disc_info_data);
>    if (opt_i_status != NULL) *opt_i_status = i_status;
>
>    return (DRIVER_OP_SUCCESS == i_status) ?
>        (((*mmc_disc_info_data)[2] & 0x03 == 0x00) ? true : false)
>        : false;
> }
>
>
> I also have one question: I have a DVD+R (Verbatim) that is not detected as
> a DVD by the cdio_is_discmode_dvd function. for this media, discmode is
> CDIO_DISCMODE_DVD_OTHER. Maybe You have a good reason to do so, but would
> it
> be possible to change this function ?
> bool
> cdio_is_discmode_dvd(discmode_t discmode)
> {
>  switch (discmode) {
>  case CDIO_DISC_MODE_DVD_ROM:
>  case CDIO_DISC_MODE_DVD_RAM:
>  case CDIO_DISC_MODE_DVD_R:
>  case CDIO_DISC_MODE_DVD_RW:
>  case CDIO_DISC_MODE_DVD_PR:
>  case CDIO_DISC_MODE_DVD_PRW:
>  case CDIO_DISC_MODE_DVD_OTHER:
>    return true;
>  default:
>    return false;
>  }
> }
>
>
> My intention is to write a simple "mmc_get_disc_information" function using
> mmc commands and cdio/iso9660 functions (cdio_get_discmode =>
> mmc_get_discmode, cdio_is_discmode_dvd, cdio_get_disc_last_lsn
> =>mmc_get_disc_last_lsn, cdio_guess_cd_type, iso9660_fs_read_pvd). Is it OK
> ? Here are the data types I propose:
> typedef enum {
>    GENERIC_DISC_MODE_ERROR, //also used when no media is detected
>    GENERIC_DISC_MODE_CD,
>    GENERIC_DISC_MODE_DVD,
>    //GENERIC_DISC_MODE_BD
> } generic_disctype_t;
>
> typedef struct {
>    track_t num;
>    cdio_fs_t fs_type;
>    uint32_t size;
> } track_info_t;
>
> typedef struct {
>    generic_disctype_t type;
>    bool empty;
>    bool rewritable;
>    uint32_t size; //0 if empty
>    uint32_t capacity; //for writable media only
>    uint8_t tracks_count;
>    track_info_t* tracks;
> } disc_information_t;
>
> As You can see, my intention is too give "High Level" information only, to
> simplify programmers work (my point of view is a burning application). I
> hope You are still interested.
>
> Best Regards !
>
>
> --
> Frank Endres
>