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

[Kevin Wolf]

Am 21.11.2015 um 01:20 hat Programmingkid geschrieben:
> 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.
> 
> Signed-off-by: John Arbuckle <address@hidden>
> 
> ---
> This patch depends on the GetBSDPath patch.
> 
>  block/raw-posix.c |   90 +++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 64 insertions(+), 26 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index ccfec1c..28bce71 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -42,9 +42,8 @@
>  #include <IOKit/storage/IOMediaBSDClient.h>
>  #include <IOKit/storage/IOMedia.h>
>  #include <IOKit/storage/IOCDMedia.h>
> -//#include <IOKit/storage/IOCDTypes.h>
>  #include <CoreFoundation/CoreFoundation.h>
> -#endif
> +#endif /* (__APPLE__) && (__MACH__) */
>  
>  #ifdef __sun__
>  #define _POSIX_PTHREAD_SEMANTICS 1
> @@ -1975,7 +1974,7 @@ BlockDriver bdrv_file = {
>  /* host device */
>  
>  #if defined(__APPLE__) && defined(__MACH__)
> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
> +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 )
> @@ -2033,7 +2032,34 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, 
> char *bsdPath,
>      return kernResult;
>  }
>  
> -#endif
> +/* Sets up a real cdrom for use in QEMU */
> +static bool 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++) {
> +        snprintf(testPartition, sizeof(testPartition), "%ss%d", bsdPath, 
> index);
> +        fd = qemu_open(testPartition, O_RDONLY | O_BINARY | O_LARGEFILE);
> +        if (fd >= 0) {
> +            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");

This should be at least error_report(), so that the message is printed
in the monitor if called in monitor context.

If this means failure to open the device, the Error object should be
used instead.

> +    } else {
> +        DPRINTF("Using %s as optical disc\n", testPartition);
> +        pstrcpy(bsdPath, MAXPATHLEN, testPartition);
> +    }
> +    return partitionFound;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>  
>  static int hdev_probe_device(const char *filename)
>  {
> @@ -2121,34 +2147,32 @@ static int hdev_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      BDRVRawState *s = bs->opaque;
>      Error *local_err = NULL;
>      int ret;
> +    bool cdromOK = true;

This is an unused variable on non-Apple platforms and breaks the build.

Also, it should be cdrom_ok to match the qemu coding style.

>  #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),
> -                                                                        
> 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';
> + 

This line adds a trailing space character.

> +    /* If using a physical device */
> +    if (strstart(filename, "/dev/", NULL)) {
> +        char bsdPath[MAXPATHLEN];
> +
> +        /* If the physical device is a cdrom */
> +        if (strcmp(filename, "/dev/cdrom") == 0) {

The original code considered everything that starts in "/dev/cdrom", but
this one only considers exact matches. Intentional?

The outer strstart() check is redundant in this code as it is written.
I'm not sure what you really wanted to do for the case that starts in
"/dev/" but is different from "/dev/cdrom", but with this implementation
nothing happens.

> +            io_iterator_t mediaIterator;
> +            FindEjectableCDMedia(&mediaIterator);
> +            GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags);
> +            if (bsdPath[0] == '\0') {
> +                printf("Error: failed to obtain bsd path for optical 
> drive!\n");

If this is really an error, shouldn't we actually set errp and return
from the function? And if it's not an error, being silent sounds right.

But if we do want to print an error, error_report() again.

>              } else {
> -                qemu_close(fd);
> +                cdromOK = setupCDROM(bsdPath);
> +                filename = bsdPath;
> +            }
> +
> +            if (mediaIterator) {
> +                IOObjectRelease(mediaIterator);
>              }
> -            filename = bsdPath;
>              qdict_put(options, "filename", qstring_from_str(filename));
>          }
> -
> -        if ( mediaIterator )
> -            IOObjectRelease( mediaIterator );
>      }
>  #endif
>  
> @@ -2159,7 +2183,21 @@ static int hdev_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          if (local_err) {
>              error_propagate(errp, local_err);
>          }
> -        return ret;
> +    }
> +
> +#if defined(__APPLE__) && defined(__MACH__)
> +    /* if a physical device experienced an error while being opened */
> +    if (strncmp(filename, "/dev/", 5) == 0 && (cdromOK == false || ret != 
> 0)) {

Using strstart() would probably be more consistent.

Also with boolean, I generally prefer !cdrom_ok, but that's a matter of
taste and up to you.

> +        printf("\nError: If device %s is mounted on the desktop, unmount it"
> +                        " first before using it in QEMU.\n", filename);
> +        printf("Command to unmount device: diskutil unmountDisk %s\n",
> +                                                                    
> filename);
> +        printf("Command to mount device: diskutil mountDisk %s\n", filename);

Probably error_report() here as well.

> +    }
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> +
> +    if (ret < 0) {
> +       return ret;
>      }

I asked in v5 whether ret > 0 was possible (because otherwise the two
'if (ret < 0)' blocks could be merged) and you said it was. Now I
reviewed raw_open_common() and I must say that I can't see how this
function would ever return anything other than 0 or a negative errno.

>      /* Since this does ioctl the device must be already opened */
> -- 
> 1.7.5.4

Kevin