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

[Daniel P. Berrange]

On Tue, Feb 02, 2016 at 12:08:31PM -0500, Programmingkid wrote:
> https://patchwork.ozlabs.org/patch/570128/
> 
> 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. Now QEMU uses both CD and DVD media.
> 
> Signed-off-by: John Arbuckle <address@hidden>
> 
> ---
> Changed filename variable to a character array.
> Changed how filename was set to bsd_path's value by using snprintf().

Whats the rationale here ? Using pre-allocated fixed
length arrays is pretty bad practice in general, but
especially so for filenames


> @@ -2119,34 +2173,59 @@ static int hdev_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      int ret;
>  
>  #if defined(__APPLE__) && defined(__MACH__)
> -    const char *filename = qdict_get_str(options, "filename");

[snip]

> +    char filename[MAXPATHLEN];
> +    bool error_occurred = false;
> +    snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));

This is a step backwards compared to the original code
IMHO. There's no good reason to use a stack allocated
fixed array here - the code that follows would be
quite happy with the original 'const char *'.

> +    /* If using a real cdrom */
> +    if (strcmp(filename, "/dev/cdrom") == 0) {
> +        char bsd_path[MAXPATHLEN];
> +        char *mediaType = NULL;
> +        kern_return_t ret_val;
> +        io_iterator_t mediaIterator = 0;
> +
> +        mediaType = FindEjectableOpticalMedia(&mediaIterator);
> +        if (mediaType == NULL) {
> +            error_setg(errp, "Please make sure your CD/DVD is in the optical"
> +                       " drive");
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
> +        }
> +
> +        ret_val = GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), 
> flags);
> +        if (ret_val != KERN_SUCCESS) {
> +            error_setg(errp, "Could not get BSD path for optical drive");
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
> +        }
> +
> +        /* If a real optical drive was not found */
> +        if (bsd_path[0] == '\0') {
> +            error_setg(errp, "Failed to obtain bsd path for optical drive");
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
> +        }
> +
> +        /* If using a cdrom disc and finding a partition on the disc failed 
> */
> +        if (strncmp(mediaType, kIOCDMediaClass, 9) == 0 &&
> +            setup_cdrom(bsd_path, errp) == false) {
> +            print_unmounting_directions(bsd_path);
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
>          }
>  
> -        if ( mediaIterator )
> -            IOObjectRelease( mediaIterator );
> +        snprintf(filename, MAXPATHLEN, "%s", bsd_path);
> +        qdict_put(options, "filename", qstring_from_str(filename));
> +
> +hdev_open_Mac_error:
> +        g_free(mediaType);
> +        if (mediaIterator) {
> +            IOObjectRelease(mediaIterator);
> +        }
> +        if (error_occurred) {
> +            return -1;
> +        }
>      }
> -#endif
> +#endif /* defined(__APPLE__) && defined(__MACH__) */

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|