Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Dec 11, 2015, at 5:00 PM, Jeff Cody wrote:

> On Thu, Dec 10, 2015 at 09:39:51AM -0500, Programmingkid wrote:
>> https://patchwork.ozlabs.org/patch/550295/
>> 
>> Mac OS X can be picky when it comes to allowing the user
>> to use physical devices in QEMU. Most mounted volumes
>> appear to be off limits to QEMU. If an issue is detected,
>> a message is displayed showing the user how to unmount a
>> volume.
>> 
> 
> This commit message doesn't seem to really match the patch; it is more
> than just a message displayed to the user.  For instance, before it
> looked for just kIOCDMediaClass - now, it searches for kIOCDMediaClass
> and kIODVDMediaClass.

I guess the commit message is a little out of date. How about this:

Mac OS X can be picky when it comes to allowing the user
to use physical devices in QEMU. Most mounted volumes
appear to be off limits to QEMU. If an issue is detected,
a message is displayed showing the user how to unmount a
volume. Also new to this patch is the ability to use DVD
disc.

> 
>> Signed-off-by: John Arbuckle <address@hidden>
>> 
>> ---
>> error_report()'s had their \n, '.', and "Error:" removed.
>> Indentations are now at the left parenthesis rather than
>> at the 80 character mark.
>> Added spaces between the + sign.
>> 
> 
> Also, this patch seems garbled.  When saved via Mutt, or when I view
> it "raw", there are '=20" and '=3D' all around, a sure sign that it is
> (or once was) not plaintext.
> 
> I recommend using git format-patch [1] and git send-email [1] to
> create and send patches - it'll do the Right Thing.

You really see that? I don't know why. The link I provide to the patch in
patchworks shows no problems. The patch itself was sent as a plain text
file.

> 
>> block/raw-posix.c |  135 
>> +++++++++++++++++++++++++++++++++++++++--------------
>> 1 files changed, 99 insertions(+), 36 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ccfec1c..39e523b 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -43,6 +43,7 @@
>> #include <IOKit/storage/IOMedia.h>
>> #include <IOKit/storage/IOCDMedia.h>
>> //#include <IOKit/storage/IOCDTypes.h>
>> +#include <IOKit/storage/IODVDMedia.h>
>> #include <CoreFoundation/CoreFoundation.h>
>> #endif
>> 
>> @@ -1975,32 +1976,46 @@ BlockDriver bdrv_file = {
>> /* host device */
>> 
>> #if defined(__APPLE__) && defined(__MACH__)
>> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
>> static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
>>                                 CFIndex maxPathSize, int flags);
>> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> +                                               char *mediaType)
>> {
>>     kern_return_t       kernResult;
>>     mach_port_t     masterPort;
>>     CFMutableDictionaryRef  classesToMatch;
>> +    const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
>> 
>>     kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort );
>>     if ( KERN_SUCCESS != kernResult ) {
>>         printf( "IOMasterPort returned %d\n", kernResult );
>>     }
>> 
>> -    classesToMatch = IOServiceMatching( kIOCDMediaClass );
>> -    if ( classesToMatch == NULL ) {
>> -        printf( "IOServiceMatching returned a NULL dictionary.\n" );
>> -    } else {
>> -    CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), 
>> kCFBooleanTrue );
>> -    }
>> -    kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, 
>> mediaIterator );
>> -    if ( KERN_SUCCESS != kernResult )
>> -    {
>> -        printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
>> -    }
>> +    int index;
>> +    for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
>> +        classesToMatch = IOServiceMatching(matching_array[index]);
>> +        if (classesToMatch == NULL) {
>> +            error_report("IOServiceMatching returned NULL for %s",
>> +                         matching_array[index]);
>> +            continue;
>> +        }
>> +        CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>> +                             kCFBooleanTrue);
>> +        kernResult = IOServiceGetMatchingServices(masterPort, 
>> classesToMatch,
>> +                                                  mediaIterator);
>> +        if (kernResult != KERN_SUCCESS) {
>> +            error_report("Note: IOServiceGetMatchingServices returned %d",
>> +                         kernResult);
>> +        }
>> 
>> +        /* If a match was found, leave the loop */
>> +        if (*mediaIterator != 0) {
> 
> Since mediaIterator was not ever initialized in hdev_open, we can't
> assume the value of mediaIterator as being 0 after kernResult !=
> KERN_SUCCESS, can we?  A quick google search [3] shows it ambiguous, so
> best initialize mediaIterator down below...

Sounds like a good suggestion.

> 
> 
>> +            DPRINTF("Matching using %s\n", matching_array[index]);
>> +            snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
>> +                     matching_array[index]);
> 
> Just use g_strdup().

I like snprintf() because it very well documented.

Is this what you had in mind:

mediaType = g_strdup(matching_array[index]);


> 
> We use snprintf here, with a size limit of the string referenced in
> the array, which references a string defined in an OS X system header...
> 
>> +            break;
>> +        }
>> +    }
>>     return kernResult;
> 
> You may return garbage here, because kernResult is never initialized,
> and your for() loop short circuits on a NULL return from
> IOServiceMatching().

Tests show the code works. I did do tests where there was no CD in the 
DVD drive. The code does gracefully handle the situation.

> Does this compile with our flags?
Yes

>  I don't have
> OS X so I can't try it, but I thought at least with gcc and -werror, a
> possible uninitialized usage would be flagged.

Didn't have any warnings or errors compiling. 

> 
>> }
>> 
>> @@ -2033,7 +2048,35 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, 
>> char *bsdPath,
>>     return kernResult;
>> }
>> 
>> -#endif
>> +/* Sets up a real cdrom for use in QEMU */
>> +static bool setup_cdrom(char *bsd_path, Error **errp)
>> +{
>> +    int index, num_of_test_partitions = 2, fd;
> 
> What is the magic of 2 test partitions?

CD's are setup one way or another. My tests show some CD's
only work when we use partition zero. Other times it is
partition one.

> 
>> +    char test_partition[MAXPATHLEN];
>> +    bool partition_found = false;
>> +
>> +    /* look for a working partition */
>> +    for (index = 0; index < num_of_test_partitions; index++) {
>> +        snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
>> +                 index);
>> +        fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
>> +        if (fd >= 0) {
>> +            partition_found = true;
>> +            qemu_close(fd);
>> +            break;
>> +        }
>> +    }
>> +
>> +    /* if a working partition on the device was not found */
>> +    if (partition_found == false) {
>> +        error_setg(errp, "Failed to find a working partition on disc");
>> +    } else {
>> +        DPRINTF("Using %s as optical disc\n", test_partition);
>> +        pstrcpy(bsd_path, MAXPATHLEN, test_partition);
> 
> In OS X, is MAXPATHLEN including the terminating NULL?

I think so. 

> 
>> +    }
>> +    return partition_found;
>> +}
>> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>> 
>> static int hdev_probe_device(const char *filename)
>> {
>> @@ -2115,6 +2158,17 @@ static bool hdev_is_sg(BlockDriverState *bs)
>>     return false;
>> }
>> 
>> +/* Prints directions on mounting and unmounting a device */
>> +static void print_unmounting_directions(const char *file_name)
>> +{
>> +    error_report("If device %s is mounted on the desktop, unmount"
>> +                 " it first before using it in QEMU", file_name);
>> +    error_report("Command to unmount device: diskutil unmountDisk %s",
>> +                 file_name);
>> +    error_report("Command to mount device: diskutil mountDisk %s",
>> +                 file_name);
>> +}
>> +
>> static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>>                      Error **errp)
>> {
>> @@ -2125,30 +2179,32 @@ static int hdev_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>> #if defined(__APPLE__) && defined(__MACH__)
>>     const char *filename = qdict_get_str(options, "filename");
>> 
>> -    if (strstart(filename, "/dev/cdrom", NULL)) {
>> -        kern_return_t kernResult;
>> +    /* If using a real cdrom */
>> +    if (strcmp(filename, "/dev/cdrom") == 0) {
>> +        char bsd_path[MAXPATHLEN];
>> +        char mediaType[11]; /* IODVDMedia is the longest value */
> 
> ... yet here we just hard code the array size to 11, which is prone to
> an error later on by either a change in the system header (not very
> likely, but possible), or by expanding the media types we support in
> the future.
> 
> Just bypass that fragility, and use g_strdup() (and later g_free()),
> so we are impervious to the exact string size.  You'll likely want a
> common exit for the g_free() call.

I will use g_strdup() on my next patch.

> 
>>         io_iterator_t mediaIterator;
> 
> ...Given your usage of mediaIterator, I think you need to initialize it
> to 0 here.

Ok

> 
>> -        char bsdPath[ MAXPATHLEN ];
>> -        int fd;
>> -
>> -        kernResult = FindEjectableCDMedia( &mediaIterator );
>> -        kernResult = GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath),
>> -                                                                        
>> flags);
>> -        if ( bsdPath[ 0 ] != '\0' ) {
>> -            strcat(bsdPath,"s0");
>> -            /* some CDs don't have a partition 0 */
>> -            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>> -            if (fd < 0) {
>> -                bsdPath[strlen(bsdPath)-1] = '1';
>> -            } else {
>> -                qemu_close(fd);
>> -            }
>> -            filename = bsdPath;
>> -            qdict_put(options, "filename", qstring_from_str(filename));
>> +        FindEjectableOpticalMedia(&mediaIterator, mediaType);
> 
> Return value ignored here, don't ignore it.

ok

> 
>> +        GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
> 
> Return value ignored here, don't ignore it.

ok

> 
>> +        if (mediaIterator) {
>> +            IOObjectRelease(mediaIterator);
>>         }
>> 
>> -        if ( mediaIterator )
>> -            IOObjectRelease( mediaIterator );
>> +        /* If a real optical drive was not found */
>> +        if (bsd_path[0] == '\0') {
>> +            error_setg(errp, "Failed to obtain bsd path for optical drive");
>> +            return -1;
>> +        }
>> +
>> +        /* If using a cdrom disc and finding a partition on the disc failed 
>> */
>> +        if (strncmp(mediaType, "IOCDMedia", 9) == 0 &&
>> +                                         setup_cdrom(bsd_path, errp) == 
>> false) {
>> +            print_unmounting_directions(bsd_path);
>> +            return -1;
>> +        }
>> +
>> +        filename = bsd_path;
>> +        qdict_put(options, "filename", qstring_from_str(filename));
>>     }
>> #endif
>> 
>> @@ -2159,9 +2215,16 @@ static int hdev_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>         if (local_err) {
>>             error_propagate(errp, local_err);
>>         }
>> -        return ret;
> 
> Spurious line delete?

It needed to go so the print_unmounting_directions()
function can have a chance to be called. I do see a
need for it in Linux, so I will bring it back in the
next patch.

> 
>>     }
>> 
>> +#if defined(__APPLE__) && defined(__MACH__)
>> +    /* if a physical device experienced an error while being opened */
>> +    if (strncmp(filename, "/dev/", 5) == 0 && ret != 0) {
>> +        print_unmounting_directions(filename);
>> +        return -1;
>> +    }
>> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>> +
>>     /* Since this does ioctl the device must be already opened */
>>     bs->sg = hdev_is_sg(bs);
>> 
>> -- 
>> 1.7.5.4
>> 
>> 
>> 
> 
> [1] https://git-scm.com/docs/git-format-patch
> [2] https://git-scm.com/docs/git-send-email
> [3] 
> https://developer.apple.com/library/mac/documentation/IOKit/Reference/IOKitLib_header_reference/index.html#//apple_ref/doc/c_ref/IOServiceGetMatchingServices


Thank you for reviewing my patch.