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

[Eric Blake <eblake@redhat.com>]

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

On 11/25/2015 05:24 PM, Programmingkid wrote:
> 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>
> 
> ---

Right here (between the --- and diffstat) it's nice to post a changelog
of how v8 differs from v7, to help earlier reviewers focus on the
improvements.

>  block/raw-posix.c |   98 +++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 72 insertions(+), 26 deletions(-)

> +++ 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__) */
>  

This hunk looks to be an unrelated cleanup; you might want to just
propose it separately through the qemu-trivial queue (but don't forget
that even trivial patches must cc qemu-devel).

> +
> +    /* look for a working partition */
> +    for (index = 0; index < num_of_test_partitions; index++) {
> +        snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
> +                                                                         index);

Unusual indentation.  More typical would be:

        snprintf(test_partition, sizeof(test_partition), "%ss%d",
                 bsd_path, index);

with the second line flush to the character after the ( of the first line.

> +        fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);

Isn't qemu_open() supposed to provide O_LARGEFILE for ALL users
automatically?  (That is, why would we ever _want_ to handle a file
using only 32-bit off_t?)  But that's a separate issue; it looks like
you are copy-and-pasting from existing use of this idiom already in
raw-posix.c.

> +        if (fd >= 0) {
> +            partition_found = true;
> +            qemu_close(fd);
> +            break;
> +        }
> +    }
> +
> +    /* if a working partition on the device was not found */
> +    if (partition_found == false) {
> +        error_setg(errp, "Error: Failed to find a working partition on "
> +                                                                     "disc!\n");

Several violations of convention.  error_setg() does not need a
redundant "Error: " prefix, should not end in '!' (we aren't shouting),
and should not end in newline.  And with those fixes, you won't even
need the weird indentation.

        error_setg(errp, "failed to find a working partition on disk");

>  
> +/* Prints directions on mounting and unmounting a device */
> +static void printUnmountingDirections(const char *file_name)

Elsewhere, we use 'function_name', not 'functionName'.

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

Again, weird indentation. And don't use \n at the end of error_report().

> @@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>      int ret;
>  
>  #if defined(__APPLE__) && defined(__MACH__)
> -    const char *filename = qdict_get_str(options, "filename");
> +    const char *file_name = qdict_get_str(options, "filename");

No need to rename this variable.

> +
> +        /* If a real optical drive was not found */
> +        if (bsd_path[0] == '\0') {
> +            error_setg(errp, "Error: failed to obtain bsd path for optical"
> +                                                                   " drive!\n");

Again, weird indentation, redundant "Error: ", and no "!\n" at the end.

>  
> +#if defined(__APPLE__) && defined(__MACH__)
> +    /* if a physical device experienced an error while being opened */
> +    if (strncmp(file_name, "/dev/", 5) == 0 && ret != 0) {
> +        printUnmountingDirections(file_name);

Is this advice appropriate to ALL things under /dev/, or just cdroms?

> +        return -1;
> +    }
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> +
>      /* Since this does ioctl the device must be already opened */
>      bs->sg = hdev_is_sg(bs);
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]