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

[Kevin Wolf]

Am 25.11.2015 um 06:43 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>
> 
> ---
> Changed cdromOK variable to cdrom_ok.
> Set initial value of cdrom_ok to false.
> Moved declaration of cdrom_ok variable to Mac OS X code block.
> Replaced printf() calls with error_report().
> Changed if (ret < 0) to if (ret != 0).
> 
>  block/raw-posix.c |  102 
> ++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index aec9ec6..3625f35 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,9 +1974,9 @@ 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 );
> -
> +static kern_return_t FindEjectableCDMedia(io_iterator_t *mediaIterator);
> +static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> +                                CFIndex maxPathSize, int flags);

You seem to have accidentally merged this with the GetBSDPath() patch I
already applied.

>  kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
>  {
>      kern_return_t       kernResult;
> @@ -2004,7 +2003,8 @@ kern_return_t FindEjectableCDMedia( io_iterator_t 
> *mediaIterator )
>      return kernResult;
>  }
>  
> -kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, 
> CFIndex maxPathSize )
> +kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> +                         CFIndex maxPathSize, int flags)
>  {
>      io_object_t     nextMedia;
>      kern_return_t   kernResult = KERN_FAILURE;
> @@ -2017,7 +2017,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;
> @@ -2030,7 +2032,34 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, 
> char *bsdPath, CFIndex ma
>      return kernResult;
>  }
>  
> -#endif
> +/* Sets up a real cdrom for use in QEMU */
> +static bool setupCDROM(char *bsdPath)

Please try to respect the qemu coding style in all new code, including
this function. That is, setup_cdrom, bsd_path, num_of_test_partitions,
etc.

> +{
> +    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) {
> +        error_report("Error: Failed to find a working partition on disc!\n");

As you said this is a real error, please pass an Error** to this
function and use error_setg() here.

> +    } 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)
>  {
> @@ -2120,32 +2149,31 @@ static int hdev_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      int ret;
>  
>  #if defined(__APPLE__) && defined(__MACH__)
> +    bool cdrom_ok = false;
>      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';
> +    /* If using a physical device */
> +    if (strstart(filename, "/dev/", NULL)) {
> +        char bsdPath[MAXPATHLEN];

This strstart() check is still redundant. Move the bsdPath declaration
inside the nested if block and get rid of the outer if.

> +        /* If the physical device is a cdrom */
> +        if (strcmp(filename, "/dev/cdrom") == 0) {
> +            io_iterator_t mediaIterator;
> +            FindEjectableCDMedia(&mediaIterator);
> +            GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags);
> +            if (bsdPath[0] == '\0') {
> +                error_report("Error: failed to obtain bsd path for optical"
> +                                                                   " 
> drive!\n");

As you said this is a real error, assign errp instead.

Also, you need to make sure that the function is actually exited. With
this patch, we print an error message, but continue and rely on
raw_open_common() to fail, which is ugly and probably results in a
suboptimal error message.

Perhaps create a fail: label and move the unmount instructions there so
you can simply goto fail here.

>              } else {
> -                qemu_close(fd);
> +                cdrom_ok = setupCDROM(bsdPath);
> +                filename = bsdPath;
> +            }
> +
> +            if (mediaIterator) {
> +                IOObjectRelease(mediaIterator);
>              }
> -            filename = bsdPath;
>              qdict_put(options, "filename", qstring_from_str(filename));
>          }
> -
> -        if ( mediaIterator )
> -            IOObjectRelease( mediaIterator );
>      }
>  #endif
>  
> @@ -2156,7 +2184,23 @@ 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 && (cdrom_ok == false || ret != 
> 0)) {

cdrom_ok is false for everything that isn't /dev/cdrom. That means that
you display the message not only for CD drives, but for all devices or
files starting with /dev; and not only in error cases, but even on
success.

> +        error_report("Error: If device %s is mounted on the desktop, unmount"
> +                              " it first before using it in QEMU.\n", 
> filename);
> +        error_report("Command to unmount device: diskutil unmountDisk %s\n",
> +                                                                      
> filename);
> +        error_report("Command to mount device: diskutil mountDisk %s\n",
> +                                                                      
> filename);
> +    }
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> +
> +    /* End execution of this function if an error took place */
> +    if (ret != 0) {
> +       return ret;

Indentation is off here (3 instead of 4 spaces).

Also, as I already commented in v6, please unify this into a single
error handling if block.

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

Kevin