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 <[email protected]>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 >
