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

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 <programmingkidx@gmail.com>

---
This patch depends on the GetBSDPath patch.

 block/raw-posix.c |   90 +++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 64 insertions(+), 26 deletions(-)

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 <programmingkidx@gmail.com>
> 
> ---
> 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

On Nov 24, 2015, at 9:38 AM, Kevin Wolf wrote:
> 
> 
>> +    /* 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?

Yes. CDROM's are handled differently from other kinds of devices. 

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

It is an error. But there is a chance that unmounting the device's volume from
the desktop might fix the problem, so letting the function continue to the helpful
error messages would be beneficial to the user. 

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

I would really prefer to stick with strncmp(). It is ANSI C and very well documented.
A search for strstart() did not turn up any documentation on it. 

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

It is possible the raw_open_common() function could be changed in the future
to produce positive error numbers. I think checking for anything that isn't zero
is the best thing to do.

Am 25.11.2015 um 05:18 hat Programmingkid geschrieben:
> 
> On Nov 24, 2015, at 9:38 AM, Kevin Wolf wrote:
> > 
> > 
> >> +    /* 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?
> 
> Yes. CDROM's are handled differently from other kinds of devices. 

I doubt that other kind of devices start in /dev/cdrom, but  on second
thought, as this isn't an actual filename, but just a magic string that
qemu converts to a real CD-ROM drive, your change makes sense.

Perhaps it should be split into its own patch, though, because it's a
logically independent change.

> > 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.
> 
> It is an error. But there is a chance that unmounting the device's volume from
> the desktop might fix the problem, so letting the function continue to the helpful
> error messages would be beneficial to the user. 

Then you need to make sure that we actually don't open the file and
instead run the error path. This patch doesn't achieve that.

> >> 
> >> +#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.
> 
> I would really prefer to stick with strncmp(). It is ANSI C and very well documented.
> A search for strstart() did not turn up any documentation on it. 

include/qemu-common.h:

    /**
     * strstart:
     * @str: string to test
     * @val: prefix string to look for
     * @ptr: NULL, or pointer to be written to indicate start of
     *       the remainder of the string
     *
     * Test whether @str starts with the prefix @val.
     * If it does (including the degenerate case where @str and @val
     * are equal) then return true. If @ptr is not NULL then a
     * pointer to the first character following the prefix is written
     * to it. If @val is not a prefix of @str then return false (and
     * @ptr is not written to).
     *
     * Returns: true if @str starts with prefix @val, false otherwise.
     */
    int strstart(const char *str, const char *val, const char **ptr);

> > 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.
> 
> It is possible the raw_open_common() function could be changed in the future
> to produce positive error numbers. I think checking for anything that isn't zero
> is the best thing to do.

Please write your code so that it makes sense today.

raw_open_common() won't ever return positive error numbers, that would
be insane and against all conventions. It's even less likely than making
0 an error return value (i.e. converting the function to return a
boolean value) - but I can't see either happening. We have reviews to
avoid such misguided changes.

The only thing that might theoretically happen is that positive values
could at some point denote different successful results. But if anyone
were to propose an insane change like that, it would be their job to
adapt the callers to the new interface.

Such a highly hypothetical change is not a reason to clutter our code
now.

Kevin