Re: [Libcdio-devel] Re: Submission of new mmc function for libcdio

[Rocky Bernstein]

Allowing NULL for the location of the status is okay, although personally as
a programmer I'd recommend against doing this.

The doxygen comment isn't updated below; also I find the way this is coded
more awkward than just saving the status to a local variable and setting the
parameter location afterwards when desired.

Please refer to what is currently checked into git which has these changes;
it also includes a test that a call with a NULL parameter doesn't SEGV as it
would have before.

In general I'd prefer patches off of the git repository as an attached file.
There are schools of thought (test-driven development, behavior-driven
development) that one the tests first, they fail (in this case via a SEGVs)
and then you write the code which stops the failure.

As for mm_disc_empty, I don't think it bad to have lots of functions like
this if that is likely how it is going to be needed inside a program.

What seems awkward is that underneath they both issue the "fat"
status-retrieving  MMC operation: READ DISC INFO. But I think of that
as a *separate
*issue to address.

Here are two possibilities to address that issue...

Define a structure to save that information which is set by one of the many
functions that need it. Much like was done to mmc_drive_erasable with its
operation-status parameter. If that structure is passed in (i.e. not NULL)
one field in the structure would indicate whether or not to use the existing
information, or ignore and it retrieve it fresh, and reset the passed
information.

As suggested earlier, a MMC library should include functions for each of the
commands that the MMC standards define. Or if not all of them (since this is
a lot of work initially) at least the common one or those we come across
often. So READ DISC INFO would probably be one of those. That would probably
return an array of bytes. So there might be another routine to parse this
array of bytes into a more programmer-friendly structure, or access routines
to pick out parts of the bytes and convert them to more programmer-friendly
data types. There are already access routines in libcdio's mmc to assist
extracting fields.

On the other end of the spectrum, one could let the MMC abstraction bleed
back into the programmer and make him/her deal with it. That is, make the
programmer issue a READ DISC INFO command beforehand and then call a routine
with the results to interpret the desired result.  Although this is what I
commonly find in code that needs information of this nature, I don't think I
like this.

As for the disc capacity, there is a MMC command READ CAPACITY:

The READ CAPACITY command provides a means for the Host to request
> information regarding the capacity of media currently loaded into the Drive.
>

And READ DISC INFORMATION which will give among other things the last track
of the last session.

But it sounds from the name of the function you give, isosize, that you want
something regarding the size of that ISO 9660 file system occupies. And here
I have to caution you regarding a common confusion. The physical medium can
contain many tracks and many sessions each of which might contain an ISO
9660 file system on it. Often one finds that a data disc has one track with
one filesystem on it - ISO 9660 for CD's and UDF for DVD's. But other
combinations are possible, as are multiple tracks and sessions.

At any rate, if you want information about a particular ISO 9660 filesystem
size, that would be part of the libiso9660 *library* bundled with the
libcdio *package*, not the libcdio library.  (The libcdio library is also
bundled with the libcdio package. I am sorry for the dual use of the name
"libcdio" which refers both to a package and a library inside that package.)


Externally available libcdio *library* routines generally begin with cdio_.
It is possible I messed up some here, but external iso9660 library routines
start, well, iso9660_ .

Inside the ISO 9660 PVD there is are fields volume_space_size and
logical_block_size. See the doxygen PVD structure in
iso9660.h<http://www.gnu.org/software/libcdio/doxygen/structiso9660__pvd__s.html>for
details.

Finally UDF, like MMC are long neglected aspects  and libraries of the
libcdio package. I put there out there in the hope others would find them
useful, get interested in them and extend them. I have never need to use
them much for myself. So don't expect too much from either in its current
state.



On Sun, Jan 31, 2010 at 7:07 AM, Frank Endres <address@hidden>wrote:

> Hi !
>
> I propose a little modification for "mmc_get_disc erasable" allowing
> transmitting "NULL" for the "i_status" parameter (for a programmer who
> doesn't need to get status information):
> /**
>   Detects if a disc (CD or DVD) is erasable or not.
>   @param p_user_data 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.
>   @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 *i_status
> ) {
>    mmc_cdb_t cdb = {{0, }};
>    uint8_t buf[42] = { 0, };
>     driver_return_code_t status_i = !0;
>
>    CDIO_MMC_SET_COMMAND (cdb.field, CDIO_MMC_GPCMD_READ_DISC_INFO);
>    CDIO_MMC_SET_READ_LENGTH8 (cdb.field, sizeof(buf));
>
>     if (i_status == NULL)
>        status_i = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                                                    sizeof(buf), &buf);
>    else
>        *i_status = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                                                    sizeof(buf), &buf);
>    if (i_status != NULL && *i_status == 0 || status_i == 0) {
>         if (buf[2] & 0x10)
>            return true;
>        else
>            return false;
>    } else
>        return false;
> }
>
>
> Then I propose two new functions, that I will probably use in
> "mmc_get_disc_information":
> /**
>  Detects if a disc (CD or DVD) is empy or not.
>  @param p_user_data 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.
>  @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 *i_status
> )
> {
>    mmc_cdb_t cdb = {{0, }};
>    uint8_t buf[42] = { 0, };
>     driver_return_code_t status_i = !0;
>
>    CDIO_MMC_SET_COMMAND (cdb.field, CDIO_MMC_GPCMD_READ_DISC_INFO);
>    CDIO_MMC_SET_READ_LENGTH8 (cdb.field, sizeof(buf));
>
>     if (i_status == NULL)
>        status_i = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                                                    sizeof(buf), &buf);
>    else
>        *i_status = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                                                    sizeof(buf), &buf);
>    if (i_status != NULL && *i_status == 0 || status_i == 0) {
>      if (buf[2] & 0x03 == 0x00)
>         return true;
>      else
>        return false;
>    } else
>       return false;
> }
>
>
> //WARNING INCOMPLETE FUNCTION:
> #define CDIO_MMC_GPCMD_READ_FORMAT_CAPACITIES 0x23
> /**
>  Detects a disc (CD or DVD) ccapacity.
>  @param p_user_data 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.
>  @return the detected disc capacity, or 0 (no media present, unknown
>  capacity or operation failure); for non writable discs, capacity is equal
> to size.
>  */
> guint32
> _mmc_get_disc_capacity ( const CdIo_t *p_cdio, driver_return_code_t
> *i_status ) {
>    int capacity;
>     mmc_cdb_t cdb = {{0, }};
>     uint8_t buf[268] = { 0, };
>    uint8_t descriptor_type;
>    uint8_t *p;
>    driver_return_code_t status_i = !0;
>
>    CDIO_MMC_SET_COMMAND (cdb.field, CDIO_MMC_GPCMD_READ_FORMAT_CAPACITIES);
>     CDIO_MMC_SET_READ_LENGTH8 (cdb.field, sizeof(buf));
>
>     if (i_status == NULL)
>        status_i = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                                                    sizeof(buf), &buf);
>    else
>        *i_status = mmc_run_cmd (p_cdio, 0, &cdb, SCSI_MMC_DATA_READ,
>                                                    sizeof(buf), &buf);
>    if (i_status != NULL && *i_status == 0 || status_i == 0) {
>        descriptor_type = buf[8] & 0x03;
>        if (descriptor_type == 0x03) //no media present or unknown capacity
>            capacity =  0;
>        else {
>            p = buf + 4;
>            capacity = CDIO_MMC_GET_LEN32 (p);
>            capacity = capacity * 2;
>        }
>    } else {
>        capacity =  0;
>    }
>
>    return capacity;
> }
>
> What do You think about them ? I am not sure it is a good idea to add too
> many functions, but I think it is better to have the much simpler code
> possible for "mmc_get_disc_information". If You want to add them, tell me
> and I will write the associated tests: do You prefer a patch for
> "test/driver/mmc.c" (based on git version) or that I send the code in a
> message ? Warning "mmc_get_media_capacity" is just a draft (it doesn't work
> with all medias types - is there a simpler way to get capacity ?).
>
> I also have a question: is there a function that gives the size of an
> iso9660 formatted media - like the isosize command
> ("cdio_get_disc_last_lsn"
> seems to give incorrect results for DVDs). If not (I haven't found one), I
> intend to add a "cdio_get_isosize" function.
>
> I have the same question for UDF formatted media; If You have suggestions
> on
> how to do this, tell me...
>
> Best regards !
>
> Frank
>