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

[Jeff Cody <jcody@redhat.com>]

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.

> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> 
> ---
> 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.

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


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

Just use g_strdup().

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().  Does this compile with our flags?  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.

>  }
>  
> @@ -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?

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

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

>          io_iterator_t mediaIterator;

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

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

> +        GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);

Return value ignored here, don't ignore it.

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

>      }
>  
> +#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