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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7038 bytes --]

On Thu, Jul 16, 2015 at 04:46:07PM -0400, Programmingkid wrote:
> @@ -2014,7 +2015,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
>          if ( bsdPathAsCFString ) {
>              size_t devPathLength;
>              strcpy( bsdPath, _PATH_DEV );
> -            strcat( bsdPath, "r" );
> +            if (flags & BDRV_O_NOCACHE) {
> +                strcat(bsdPath, "r");
> +            }
>              devPathLength = strlen( bsdPath );
>              if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
>                  kernResult = KERN_SUCCESS;

Is this the fix that makes CD-ROM passthrough work for you?

Does the guest boot successfully when you do:

  -drive if=ide,media=cdrom,cache=none,file=/dev/cdrom

?

> @@ -2027,7 +2030,67 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
>      return kernResult;
>  }
>  
> -#endif
> +/* Sets up a physical device for use in QEMU */
> +static void setupDevice(const char *bsdPath)
> +{
> +   /*
> +    * Mac OS X does not like allowing QEMU to use physical devices that are
> +    * mounted. Attempts to do so result in 'Resource busy' errors.
> +    */
> +
> +    int fd;
> +    fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
> +
> +    /* if the device fails to open */
> +    if (fd < 0) {
> +        printf("Error: failed to open %s\n", bsdPath);
> +        printf("If device %s is mounted on the desktop, unmount it"
> +               " first before using it in QEMU.\n", bsdPath);
> +        printf("\nCommand to unmount device: diskutil unmountDisk %s", bsdPath);
> +        printf("\nCommand to mount device: diskutil mountDisk %s\n\n", bsdPath);
> +    }

This error message is printed regardless of the errno value.  What is
the specific errno value when open(2) fails because the device is
mounted?

Also, can you move this after the raw_open_common() call to avoid the
setupCDROM()/setupDevice() changes made in this patch and duplicating
the error message.  It doesn't seem necessary to open the files ahead of
raw_open_common() since the code continues in the error case anyway:

    ret = raw_open_common(bs, options, flags, 0, &local_err);
    if (ret < 0) {
        if (local_err) {
            error_propagate(errp, local_err);
        }
#if defined(__APPLE__) && defined(__MACH__)
        if (strstart(filename, "/dev/") == 0 && ret == -EBUSY) { /* or whatever */
	    error_report("If device %s is mounted on the desktop, unmount it"
	                 " first before using it in QEMU.", bsdPath);
	    error_report("Command to unmount device: diskutil unmountDisk %s", bsdPath);
	    error_report("Command to mount device: diskutil mountDisk %s", bsdPath);
	}
#endif /* defined(__APPLE__) && defined(__MACH__) */
        return ret;
    }

That's a much smaller change.

> +
> +    /* if the device opens */
> +    else {
> +        qemu_close(fd);
> +    }
> +}
> +
> +/* Sets up a real cdrom for use in QEMU */
> +static void setupCDROM(char *bsdPath)
> +{
> +    int index, numOfTestPartitions = 2, fd;
> +    char testPartition[MAXPATHLEN];
> +    bool partitionFound = false;
> +
> +    /* look for a working partition */
> +    for (index = 0; index < numOfTestPartitions; index++) {
> +        strncpy(testPartition, bsdPath, MAXPATHLEN);

The safe way to use strncpy() is:

  strncpy(testPartition, bsdPath, MAXPATHLEN - 1);
  testPartition[MAXPATHLEN - 1] = '\0';

but pstrcpy() is a easier to use correctly.  Please use that instead.

> +        snprintf(testPartition, MAXPATHLEN, "%ss%d", testPartition, index);
> +        fd = qemu_open(testPartition, O_RDONLY | O_BINARY | O_LARGEFILE);
> +        if (fd > 0) {

Should be fd >= 0 since fd = 0 is valid too, although unlikely.

> +            partitionFound = true;
> +            qemu_close(fd);
> +            break;
> +        }
> +    }
> +
> +    /* if a working partition on the device was not found */
> +    if (partitionFound == false) {
> +        printf("Error: Failed to find a working partition on disc!\n");
> +        printf("If your disc is mounted on the desktop, trying unmounting it"
> +               " first before using it in QEMU.\n");
> +        printf("\nCommand to unmount disc: "
> +                "diskutil unmountDisk %s\n", bsdPath);
> +        printf("Command to mount disc: "
> +               "diskutil mountDisk %s\n\n", bsdPath);
> +    }
> +
> +    DPRINTF("Using %s as CDROM\n", testPartition);
> +    strncpy(bsdPath, testPartition, MAXPATHLEN);

Please use pstrcpy().

> +}
> +
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>  
>  static int hdev_probe_device(const char *filename)
>  {
> @@ -2119,30 +2182,28 @@ 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;
> -        io_iterator_t mediaIterator;
> -        char bsdPath[ MAXPATHLEN ];
> -        int fd;
> -
> -        kernResult = FindEjectableCDMedia( &mediaIterator );
> -        kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath ) );
> -
> -        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);
> +    /* If using a physical device */
> +    if (strstart(filename, "/dev/", NULL)) {
> +
> +        /* If the physical device is a cdrom */
> +        if (strcmp(filename, "/dev/cdrom") == 0) {
> +            char bsdPath[MAXPATHLEN];
> +            io_iterator_t mediaIterator;
> +            FindEjectableCDMedia(&mediaIterator);
> +            GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags);

Why did you remove the if (bsdPath[0] != [0]) check?  Now the code
ignored GetBSDPath() failures.

> +            if (mediaIterator) {
> +                IOObjectRelease(mediaIterator);
>              }
> +            setupCDROM(bsdPath);
>              filename = bsdPath;
> -            qdict_put(options, "filename", qstring_from_str(filename));
>          }

bsdPath is out of scope here so filename is a dangling pointer in the
/dev/cdrom case.

>  
> -        if ( mediaIterator )
> -            IOObjectRelease( mediaIterator );
> +        /* Setup any other physical device e.g. USB flash drive */
> +        else {
> +            setupDevice(filename);
> +        }
> +
> +        qdict_put(options, "filename", qstring_from_str(filename));
>      }
>  #endif
>  
> -- 
> 1.7.5.4
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]