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

[Programmingkid]

On Jul 17, 2015, at 9:41 AM, Stefan Hajnoczi wrote:

> 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

The guest fails during the boot process with the above command line. 


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

I could change the patch to take into account the errno value. 

errno = 2 when it fails. strerror(errno) = "No such file or directory". 

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

I will see what I can do. 

> 
>> +
>> +    /* 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.

Is pstrcpy() ansi c? I'm having trouble finding documentation for it. 

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

Ok, I will put that back in. 

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

Good catch. Will fix that.