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

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

[Programmingkid]

https://patchwork.ozlabs.org/patch/570128/

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. Now QEMU uses both CD and DVD media.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
Added continue statement to the kernResult != KERN_SUCCESS if condition in 
FindEjectableCDMedia().
Moved print_unmounting_directions() to only compile under Mac OS X. 
Fixed indentation of "setup_cdrom(bsd_path, errp) == false) {".
Replaced IOCDMedia with kIOCDMediaClass.
Changed filename variable to a character array.
Changed how filename was set to bsd_path's value by using snprintf().
Removed "goto continue_as_normal" code.
Added error_occurred variable.
Moved "if (strncmp(filename, "/dev/", 5) == 0 && ret != 0)" code to inside of 
"if (ret < 0)" block. 

 block/raw-posix.c |  169 ++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 127 insertions(+), 42 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 076d070..67dc166 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -43,6 +43,7 @@
 #include <IOKit/storage/IOMedia.h>
 #include <IOKit/storage/IOCDMedia.h>
 //#include <IOKit/storage/IOCDTypes.h>
+#include <IOKit/storage/IODVDMedia.h>
 #include <CoreFoundation/CoreFoundation.h>
 #endif
 
@@ -1971,33 +1972,47 @@ 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, int flags);
-kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
+static char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
 {
-    kern_return_t       kernResult;
+    kern_return_t kernResult = KERN_FAILURE;
     mach_port_t     masterPort;
     CFMutableDictionaryRef  classesToMatch;
+    const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
+    char *mediaType = NULL;
 
     kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort );
     if ( KERN_SUCCESS != kernResult ) {
         printf( "IOMasterPort returned %d\n", kernResult );
     }
 
-    classesToMatch = IOServiceMatching( kIOCDMediaClass );
-    if ( classesToMatch == NULL ) {
-        printf( "IOServiceMatching returned a NULL dictionary.\n" );
-    } else {
-    CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), kCFBooleanTrue );
-    }
-    kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, mediaIterator );
-    if ( KERN_SUCCESS != kernResult )
-    {
-        printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
-    }
+    int index;
+    for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
+        classesToMatch = IOServiceMatching(matching_array[index]);
+        if (classesToMatch == NULL) {
+            error_report("IOServiceMatching returned NULL for %s",
+                         matching_array[index]);
+            continue;
+        }
+        CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
+                             kCFBooleanTrue);
+        kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
+                                                  mediaIterator);
+        if (kernResult != KERN_SUCCESS) {
+            error_report("Note: IOServiceGetMatchingServices returned %d",
+                         kernResult);
+            continue;
+        }
 
-    return kernResult;
+        /* If a match was found, leave the loop */
+        if (*mediaIterator != 0) {
+            DPRINTF("Matching using %s\n", matching_array[index]);
+            mediaType = g_strdup(matching_array[index]);
+            break;
+        }
+    }
+    return mediaType;
 }
 
 kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
@@ -2029,7 +2044,46 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
     return kernResult;
 }
 
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setup_cdrom(char *bsd_path, Error **errp)
+{
+    int index, num_of_test_partitions = 2, fd;
+    char test_partition[MAXPATHLEN];
+    bool partition_found = false;
+
+    /* 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);
+        fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+        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, "Failed to find a working partition on disc");
+    } else {
+        DPRINTF("Using %s as optical disc\n", test_partition);
+        pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+    }
+    return partition_found;
+}
+
+/* Prints directions on mounting and unmounting a device */
+static void print_unmounting_directions(const char *file_name)
+{
+    error_report("If device %s is mounted on the desktop, unmount"
+                 " it first before using it in QEMU", file_name);
+    error_report("Command to unmount device: diskutil unmountDisk %s",
+                 file_name);
+    error_report("Command to mount device: diskutil mountDisk %s", file_name);
+}
+
+#endif /* defined(__APPLE__) && defined(__MACH__) */
 
 static int hdev_probe_device(const char *filename)
 {
@@ -2119,34 +2173,59 @@ 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");
-
-    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';
-            } else {
-                qemu_close(fd);
-            }
-            filename = bsdPath;
-            qdict_put(options, "filename", qstring_from_str(filename));
+    char filename[MAXPATHLEN];
+    bool error_occurred = false;
+    snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
+    /* If using a real cdrom */
+    if (strcmp(filename, "/dev/cdrom") == 0) {
+        char bsd_path[MAXPATHLEN];
+        char *mediaType = NULL;
+        kern_return_t ret_val;
+        io_iterator_t mediaIterator = 0;
+
+        mediaType = FindEjectableOpticalMedia(&mediaIterator);
+        if (mediaType == NULL) {
+            error_setg(errp, "Please make sure your CD/DVD is in the optical"
+                       " drive");
+            error_occurred = true;
+            goto hdev_open_Mac_error;
+        }
+
+        ret_val = GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
+        if (ret_val != KERN_SUCCESS) {
+            error_setg(errp, "Could not get BSD path for optical drive");
+            error_occurred = true;
+            goto hdev_open_Mac_error;
+        }
+
+        /* If a real optical drive was not found */
+        if (bsd_path[0] == '\0') {
+            error_setg(errp, "Failed to obtain bsd path for optical drive");
+            error_occurred = true;
+            goto hdev_open_Mac_error;
+        }
+
+        /* If using a cdrom disc and finding a partition on the disc failed */
+        if (strncmp(mediaType, kIOCDMediaClass, 9) == 0 &&
+            setup_cdrom(bsd_path, errp) == false) {
+            print_unmounting_directions(bsd_path);
+            error_occurred = true;
+            goto hdev_open_Mac_error;
         }
 
-        if ( mediaIterator )
-            IOObjectRelease( mediaIterator );
+        snprintf(filename, MAXPATHLEN, "%s", bsd_path);
+        qdict_put(options, "filename", qstring_from_str(filename));
+
+hdev_open_Mac_error:
+        g_free(mediaType);
+        if (mediaIterator) {
+            IOObjectRelease(mediaIterator);
+        }
+        if (error_occurred) {
+            return -1;
+        }
     }
-#endif
+#endif /* defined(__APPLE__) && defined(__MACH__) */
 
     s->type = FTYPE_FILE;
 
@@ -2155,7 +2234,13 @@ 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) {
+            print_unmounting_directions(filename);
+            return -1;
+        }
+#endif /* defined(__APPLE__) && defined(__MACH__) */
     }
 
     /* Since this does ioctl the device must be already opened */
-- 
1.7.5.4

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

[Daniel P. Berrange]

On Tue, Feb 02, 2016 at 12:08:31PM -0500, Programmingkid wrote:
> https://patchwork.ozlabs.org/patch/570128/
> 
> 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. Now QEMU uses both CD and DVD media.
> 
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> 
> ---
> Changed filename variable to a character array.
> Changed how filename was set to bsd_path's value by using snprintf().

Whats the rationale here ? Using pre-allocated fixed
length arrays is pretty bad practice in general, but
especially so for filenames


> @@ -2119,34 +2173,59 @@ 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");

[snip]

> +    char filename[MAXPATHLEN];
> +    bool error_occurred = false;
> +    snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));

This is a step backwards compared to the original code
IMHO. There's no good reason to use a stack allocated
fixed array here - the code that follows would be
quite happy with the original 'const char *'.

> +    /* If using a real cdrom */
> +    if (strcmp(filename, "/dev/cdrom") == 0) {
> +        char bsd_path[MAXPATHLEN];
> +        char *mediaType = NULL;
> +        kern_return_t ret_val;
> +        io_iterator_t mediaIterator = 0;
> +
> +        mediaType = FindEjectableOpticalMedia(&mediaIterator);
> +        if (mediaType == NULL) {
> +            error_setg(errp, "Please make sure your CD/DVD is in the optical"
> +                       " drive");
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
> +        }
> +
> +        ret_val = GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
> +        if (ret_val != KERN_SUCCESS) {
> +            error_setg(errp, "Could not get BSD path for optical drive");
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
> +        }
> +
> +        /* If a real optical drive was not found */
> +        if (bsd_path[0] == '\0') {
> +            error_setg(errp, "Failed to obtain bsd path for optical drive");
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
> +        }
> +
> +        /* If using a cdrom disc and finding a partition on the disc failed */
> +        if (strncmp(mediaType, kIOCDMediaClass, 9) == 0 &&
> +            setup_cdrom(bsd_path, errp) == false) {
> +            print_unmounting_directions(bsd_path);
> +            error_occurred = true;
> +            goto hdev_open_Mac_error;
>          }
>  
> -        if ( mediaIterator )
> -            IOObjectRelease( mediaIterator );
> +        snprintf(filename, MAXPATHLEN, "%s", bsd_path);
> +        qdict_put(options, "filename", qstring_from_str(filename));
> +
> +hdev_open_Mac_error:
> +        g_free(mediaType);
> +        if (mediaIterator) {
> +            IOObjectRelease(mediaIterator);
> +        }
> +        if (error_occurred) {
> +            return -1;
> +        }
>      }
> -#endif
> +#endif /* defined(__APPLE__) && defined(__MACH__) */

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Programmingkid]

On Feb 2, 2016, at 12:16 PM, Daniel P. Berrange wrote:

> On Tue, Feb 02, 2016 at 12:08:31PM -0500, Programmingkid wrote:
>> https://patchwork.ozlabs.org/patch/570128/
>> 
>> 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. Now QEMU uses both CD and DVD media.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> 
>> ---
>> Changed filename variable to a character array.
>> Changed how filename was set to bsd_path's value by using snprintf().
> 
> Whats the rationale here ? Using pre-allocated fixed
> length arrays is pretty bad practice in general, but
> especially so for filenames

With an automatic variable there is no worry about when to release it. 

> 
> 
>> @@ -2119,34 +2173,59 @@ 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");
> 
> [snip]
> 
>> +    char filename[MAXPATHLEN];
>> +    bool error_occurred = false;
>> +    snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
> 
> This is a step backwards compared to the original code
> IMHO. There's no good reason to use a stack allocated
> fixed array here - the code that follows would be
> quite happy with the original 'const char *'.

Having to decide when and where to free memory is eliminated with automatic variables. The code looks cleaner without all the g_free()'s. It also might eliminate possible memory leaks.

> 
>> +    /* If using a real cdrom */
>> +    if (strcmp(filename, "/dev/cdrom") == 0) {
>> +        char bsd_path[MAXPATHLEN];
>> +        char *mediaType = NULL;
>> +        kern_return_t ret_val;
>> +        io_iterator_t mediaIterator = 0;
>> +
>> +        mediaType = FindEjectableOpticalMedia(&mediaIterator);
>> +        if (mediaType == NULL) {
>> +            error_setg(errp, "Please make sure your CD/DVD is in the optical"
>> +                       " drive");
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>> +        }
>> +
>> +        ret_val = GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
>> +        if (ret_val != KERN_SUCCESS) {
>> +            error_setg(errp, "Could not get BSD path for optical drive");
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>> +        }
>> +
>> +        /* If a real optical drive was not found */
>> +        if (bsd_path[0] == '\0') {
>> +            error_setg(errp, "Failed to obtain bsd path for optical drive");
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>> +        }
>> +
>> +        /* If using a cdrom disc and finding a partition on the disc failed */
>> +        if (strncmp(mediaType, kIOCDMediaClass, 9) == 0 &&
>> +            setup_cdrom(bsd_path, errp) == false) {
>> +            print_unmounting_directions(bsd_path);
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>>         }
>> 
>> -        if ( mediaIterator )
>> -            IOObjectRelease( mediaIterator );
>> +        snprintf(filename, MAXPATHLEN, "%s", bsd_path);
>> +        qdict_put(options, "filename", qstring_from_str(filename));
>> +
>> +hdev_open_Mac_error:
>> +        g_free(mediaType);
>> +        if (mediaIterator) {
>> +            IOObjectRelease(mediaIterator);
>> +        }
>> +        if (error_occurred) {
>> +            return -1;
>> +        }
>>     }
>> -#endif
>> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> 
> Regards,
> Daniel

Thank you for your input.

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

[Daniel P. Berrange]

On Tue, Feb 02, 2016 at 12:28:24PM -0500, Programmingkid wrote:
> 
> On Feb 2, 2016, at 12:16 PM, Daniel P. Berrange wrote:
> 
> > On Tue, Feb 02, 2016 at 12:08:31PM -0500, Programmingkid wrote:
> >> https://patchwork.ozlabs.org/patch/570128/
> >> 
> >> 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. Now QEMU uses both CD and DVD media.
> >> 
> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> >> 
> >> ---
> >> Changed filename variable to a character array.
> >> Changed how filename was set to bsd_path's value by using snprintf().
> > 
> > Whats the rationale here ? Using pre-allocated fixed
> > length arrays is pretty bad practice in general, but
> > especially so for filenames
> 
> With an automatic variable there is no worry about when to release it. 
> 
> > 
> > 
> >> @@ -2119,34 +2173,59 @@ 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");
> > 
> > [snip]
> > 
> >> +    char filename[MAXPATHLEN];
> >> +    bool error_occurred = false;
> >> +    snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
> > 
> > This is a step backwards compared to the original code
> > IMHO. There's no good reason to use a stack allocated
> > fixed array here - the code that follows would be
> > quite happy with the original 'const char *'.
> 
> Having to decide when and where to free memory is eliminated with
> automatic variables. The code looks cleaner without all the g_free()'s.
> It also might eliminate possible memory leaks.

There was/is no leak because it qdict_get_str() returns 'const char *' and
so nothing needs freeing. So your change is still a backwards steps IMHO.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Programmingkid]

On Feb 2, 2016, at 12:31 PM, Daniel P. Berrange wrote:

> On Tue, Feb 02, 2016 at 12:28:24PM -0500, Programmingkid wrote:
>> 
>> On Feb 2, 2016, at 12:16 PM, Daniel P. Berrange wrote:
>> 
>>> On Tue, Feb 02, 2016 at 12:08:31PM -0500, Programmingkid wrote:
>>>> https://patchwork.ozlabs.org/patch/570128/
>>>> 
>>>> 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. Now QEMU uses both CD and DVD media.
>>>> 
>>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>>> 
>>>> ---
>>>> Changed filename variable to a character array.
>>>> Changed how filename was set to bsd_path's value by using snprintf().
>>> 
>>> Whats the rationale here ? Using pre-allocated fixed
>>> length arrays is pretty bad practice in general, but
>>> especially so for filenames
>> 
>> With an automatic variable there is no worry about when to release it. 
>> 
>>> 
>>> 
>>>> @@ -2119,34 +2173,59 @@ 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");
>>> 
>>> [snip]
>>> 
>>>> +    char filename[MAXPATHLEN];
>>>> +    bool error_occurred = false;
>>>> +    snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
>>> 
>>> This is a step backwards compared to the original code
>>> IMHO. There's no good reason to use a stack allocated
>>> fixed array here - the code that follows would be
>>> quite happy with the original 'const char *'.
>> 
>> Having to decide when and where to free memory is eliminated with
>> automatic variables. The code looks cleaner without all the g_free()'s.
>> It also might eliminate possible memory leaks.
> 
> There was/is no leak because it qdict_get_str() returns 'const char *' and
> so nothing needs freeing. So your change is still a backwards steps IMHO.

char filename[MAXPATHLEN];
snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));

So you want the above to be changed so that it goes back to this:

const char *filename = qdict_get_str(options, "filename");

then this code would have to be changed:

snprintf(filename, MAXPATHLEN, "%s", bsd_path);
qdict_put(options, "filename", qstring_from_str(filename));

I could change it to this:

qdict_put(options, "filename", qstring_from_str(bsd_path));

If I did that, then this part would not be accurate anymore:

if (strncmp(filename, "/dev/", 5) == 0) {
            print_unmounting_directions(filename);
            return -1;
        }

filename would be just "/dev/cdrom" for when the user uses the optical drive. This would print incorrect unmounting directions. 

I could add another variable that keeps track of the real device name, but that would consume more memory. It is so much easier and simpler to just use the fixed array.

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

[Eric Blake]

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

On 02/02/2016 12:10 PM, Programmingkid wrote:

>> There was/is no leak because it qdict_get_str() returns 'const char *' and
>> so nothing needs freeing. So your change is still a backwards steps IMHO.
> 
> char filename[MAXPATHLEN];
> snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
> 
> So you want the above to be changed so that it goes back to this:
> 
> const char *filename = qdict_get_str(options, "filename");

Correct.

> 
> then this code would have to be changed:
> 
> snprintf(filename, MAXPATHLEN, "%s", bsd_path);
> qdict_put(options, "filename", qstring_from_str(filename));
> 
> I could change it to this:
> 
> qdict_put(options, "filename", qstring_from_str(bsd_path));

Correct.

> 
> If I did that, then this part would not be accurate anymore:
> 
> if (strncmp(filename, "/dev/", 5) == 0) {
>             print_unmounting_directions(filename);
>             return -1;
>         }
> 
> filename would be just "/dev/cdrom" for when the user uses the optical drive. This would print incorrect unmounting directions. 

So fix it to call print_unmounting_directions(bsd_path), after hoisting
the declaration of bsd_path to be earlier to have a long enough scope.

> 
> I could add another variable that keeps track of the real device name, but that would consume more memory. It is so much easier and simpler to just use the fixed array.

And why isn't bsd_path usable for that purpose?

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

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

[Eric Blake]

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

On 02/02/2016 10:28 AM, Programmingkid wrote:

>> Whats the rationale here ? Using pre-allocated fixed
>> length arrays is pretty bad practice in general, but
>> especially so for filenames
> 
> With an automatic variable there is no worry about when to release it. 

Yeah, but it comes with the downside of having to worry about exhausting
stack space (there are platforms where MAXPATHLEN is intentionally
undefined [hello, GNU Hurd]), or with the downside of arbitrary
limitations.  And at the same time, MAXPATHLEN is usually wasteful -
allocating 1k or 4k of stack to store what is typically less than 100
bytes is dumb.  Really, storing file names in fixed length arrays is
better off to avoid, and just get used to dynamic management.

> Having to decide when and where to free memory is eliminated with automatic variables.

That's a crutch.  Anyone who can't properly manage dynamic memory
allocation, and sticks to only static allocation out of convenience, is
a rather lazy programmer.

> The code looks cleaner without all the g_free()'s. It also might eliminate possible memory leaks.

Whether eliminating g_free() looks cleaner is subjective (I actually
find g_free cleaner than foo[MAXPATHLEN]).  And the whole point of code
review is to let others ensure you don't introduce memory leaks.

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

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

[Programmingkid]

On Feb 2, 2016, at 2:24 PM, Eric Blake wrote:

> On 02/02/2016 12:10 PM, Programmingkid wrote:
> 
>>> There was/is no leak because it qdict_get_str() returns 'const char *' and
>>> so nothing needs freeing. So your change is still a backwards steps IMHO.
>> 
>> char filename[MAXPATHLEN];
>> snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
>> 
>> So you want the above to be changed so that it goes back to this:
>> 
>> const char *filename = qdict_get_str(options, "filename");
> 
> Correct.
> 
>> 
>> then this code would have to be changed:
>> 
>> snprintf(filename, MAXPATHLEN, "%s", bsd_path);
>> qdict_put(options, "filename", qstring_from_str(filename));
>> 
>> I could change it to this:
>> 
>> qdict_put(options, "filename", qstring_from_str(bsd_path));
> 
> Correct.
> 
>> 
>> If I did that, then this part would not be accurate anymore:
>> 
>> if (strncmp(filename, "/dev/", 5) == 0) {
>>            print_unmounting_directions(filename);
>>            return -1;
>>        }
>> 
>> filename would be just "/dev/cdrom" for when the user uses the optical drive. This would print incorrect unmounting directions. 
> 
> So fix it to call print_unmounting_directions(bsd_path), after hoisting
> the declaration of bsd_path to be earlier to have a long enough scope.

ok

> 
>> 
>> I could add another variable that keeps track of the real device name, but that would consume more memory. It is so much easier and simpler to just use the fixed array.
> 
> And why isn't bsd_path usable for that purpose?

bsd_path can be used for that purpose. 

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

[Programmingkid]

On Feb 2, 2016, at 2:24 PM, Eric Blake wrote:

> On 02/02/2016 12:10 PM, Programmingkid wrote:
> 
>>> There was/is no leak because it qdict_get_str() returns 'const char *' and
>>> so nothing needs freeing. So your change is still a backwards steps IMHO.
>> 
>> char filename[MAXPATHLEN];
>> snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));
>> 
>> So you want the above to be changed so that it goes back to this:
>> 
>> const char *filename = qdict_get_str(options, "filename");
> 
> Correct.
> 
>> 
>> then this code would have to be changed:
>> 
>> snprintf(filename, MAXPATHLEN, "%s", bsd_path);
>> qdict_put(options, "filename", qstring_from_str(filename));
>> 
>> I could change it to this:
>> 
>> qdict_put(options, "filename", qstring_from_str(bsd_path));
> 
> Correct.
> 
>> 
>> If I did that, then this part would not be accurate anymore:
>> 
>> if (strncmp(filename, "/dev/", 5) == 0) {
>>            print_unmounting_directions(filename);
>>            return -1;
>>        }
>> 
>> filename would be just "/dev/cdrom" for when the user uses the optical drive. This would print incorrect unmounting directions. 
> 
> So fix it to call print_unmounting_directions(bsd_path), after hoisting
> the declaration of bsd_path to be earlier to have a long enough scope.
> 
>> 
>> I could add another variable that keeps track of the real device name, but that would consume more memory. It is so much easier and simpler to just use the fixed array.
> 
> And why isn't bsd_path usable for that purpose?

After trying it out, I found out why bsd_path isn't usable for that purpose. It is because the user might try to use a flash drive as the the cdrom. Say a flash drive is set to /dev/disk2s9. If the user issues the monitor command "change ide1-cd0 /dev/disk2s9", this will make "if (strcmp(filename, "/dev/cdrom") == 0)" false and bsd_path would never be set. bsd_path contents would be garbage.

This would lead to this code not printing the unmounting directions:

if (strncmp(filename, "/dev/", 5) == 0) {
           print_unmounting_directions(filename);
           return -1;
       }

It looks keeping filename as an character array is best.

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

[Eric Blake]

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

On 02/02/2016 02:23 PM, Programmingkid wrote:

>> And why isn't bsd_path usable for that purpose?
> 
> After trying it out, I found out why bsd_path isn't usable for that purpose. It is because the user might try to use a flash drive as the the cdrom. Say a flash drive is set to /dev/disk2s9. If the user issues the monitor command "change ide1-cd0 /dev/disk2s9", this will make "if (strcmp(filename, "/dev/cdrom") == 0)" false and bsd_path would never be set. bsd_path contents would be garbage.
> 
> This would lead to this code not printing the unmounting directions:
> 
> if (strncmp(filename, "/dev/", 5) == 0) {
>            print_unmounting_directions(filename);
>            return -1;
>        }
> 
> It looks keeping filename as an character array is best.

No, keep filename as a const char * pointer.  It's easy to avoid use of
uninitialized memory.  Try this:

const char *filename;
char bsd_path[MAXPATHLEN] = "";
...
if (strncmp("/dev/cdrom"...) {
    bsd_path = ...
}
...
if (strncmp("/dev/"...) {
    print_unmounting_directions(*bsd_path ? bsd_path : filename);

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

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

[Programmingkid]

On Feb 2, 2016, at 5:04 PM, Eric Blake wrote:

> On 02/02/2016 02:23 PM, Programmingkid wrote:
> 
>>> And why isn't bsd_path usable for that purpose?
>> 
>> After trying it out, I found out why bsd_path isn't usable for that purpose. It is because the user might try to use a flash drive as the the cdrom. Say a flash drive is set to /dev/disk2s9. If the user issues the monitor command "change ide1-cd0 /dev/disk2s9", this will make "if (strcmp(filename, "/dev/cdrom") == 0)" false and bsd_path would never be set. bsd_path contents would be garbage.
>> 
>> This would lead to this code not printing the unmounting directions:
>> 
>> if (strncmp(filename, "/dev/", 5) == 0) {
>>           print_unmounting_directions(filename);
>>           return -1;
>>       }
>> 
>> It looks keeping filename as an character array is best.
> 
> No, keep filename as a const char * pointer.  It's easy to avoid use of
> uninitialized memory.  Try this:
> 
> const char *filename;
> char bsd_path[MAXPATHLEN] = "";
> ...
> if (strncmp("/dev/cdrom"...) {
>    bsd_path = ...
> }
> ...
> if (strncmp("/dev/"...) {
>    print_unmounting_directions(*bsd_path ? bsd_path : filename);

At first I thought this code looked unusual, but it does work. Is this ok?

#if defined(__APPLE__) && defined(__MACH__)
        /* if a physical device experienced an error while being opened */
        if (strncmp((*bsd_path ? bsd_path : filename), "/dev/", 5) == 0) {
            print_unmounting_directions(*bsd_path ? bsd_path : filename);
            return -1;
        }
#endif /* defined(__APPLE__) && defined(__MACH__) */

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

[Eric Blake]

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

On 02/02/2016 06:15 PM, Programmingkid wrote:

>> No, keep filename as a const char * pointer.  It's easy to avoid use of
>> uninitialized memory.  Try this:
>>
>> const char *filename;
>> char bsd_path[MAXPATHLEN] = "";
>> ...
>> if (strncmp("/dev/cdrom"...) {
>>    bsd_path = ...
>> }
>> ...
>> if (strncmp("/dev/"...) {
>>    print_unmounting_directions(*bsd_path ? bsd_path : filename);
> 
> At first I thought this code looked unusual, but it does work. Is this ok?
> 
> #if defined(__APPLE__) && defined(__MACH__)
>         /* if a physical device experienced an error while being opened */
>         if (strncmp((*bsd_path ? bsd_path : filename), "/dev/", 5) == 0) {
>             print_unmounting_directions(*bsd_path ? bsd_path : filename);
>             return -1;
>         }

A bit repetitive. You don't use filename after the fact, so shorter
would be:

#if defined(__APPLE__)...
    if (*bsd_path) {
        filename = filename;
    if (strncmp(filename, "/dev/"...) {
        print...(filename);

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

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

[Programmingkid]

On Feb 2, 2016, at 9:30 PM, Eric Blake wrote:

> On 02/02/2016 06:15 PM, Programmingkid wrote:
> 
>>> No, keep filename as a const char * pointer.  It's easy to avoid use of
>>> uninitialized memory.  Try this:
>>> 
>>> const char *filename;
>>> char bsd_path[MAXPATHLEN] = "";
>>> ...
>>> if (strncmp("/dev/cdrom"...) {
>>>   bsd_path = ...
>>> }
>>> ...
>>> if (strncmp("/dev/"...) {
>>>   print_unmounting_directions(*bsd_path ? bsd_path : filename);
>> 
>> At first I thought this code looked unusual, but it does work. Is this ok?
>> 
>> #if defined(__APPLE__) && defined(__MACH__)
>>        /* if a physical device experienced an error while being opened */
>>        if (strncmp((*bsd_path ? bsd_path : filename), "/dev/", 5) == 0) {
>>            print_unmounting_directions(*bsd_path ? bsd_path : filename);
>>            return -1;
>>        }
> 
> A bit repetitive. You don't use filename after the fact, so shorter
> would be:
> 
> #if defined(__APPLE__)...
>    if (*bsd_path) {
>        filename = filename;
>    if (strncmp(filename, "/dev/"...) {
>        print...(filename);
> 

Is this what you mean:

#if defined(__APPLE__) && defined(__MACH__)
        /* if a physical device experienced an error while being opened */
        filename = (*bsd_path ? bsd_path : filename);
        if (strncmp(filename, "/dev/", 5) == 0) {
            print_unmounting_directions(filename);
            return -1;
        }
#endif /* defined(__APPLE__) && defined(__MACH__) */

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

[Eric Blake]

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

On 02/02/2016 09:21 PM, Programmingkid wrote:

>>> #if defined(__APPLE__) && defined(__MACH__)
>>>        /* if a physical device experienced an error while being opened */
>>>        if (strncmp((*bsd_path ? bsd_path : filename), "/dev/", 5) == 0) {
>>>            print_unmounting_directions(*bsd_path ? bsd_path : filename);
>>>            return -1;
>>>        }
>>
>> A bit repetitive. You don't use filename after the fact, so shorter
>> would be:
>>
>> #if defined(__APPLE__)...
>>    if (*bsd_path) {
>>        filename = filename;

Oops, I shouldn't be writing emails late at night.  Let's try this again.

if (*bsd_path) {
    filename = bsd_path;
}
if (strncmp(filename, ...

Hopefully that makes more sense.

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

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

[Kevin Wolf]

Am 02.02.2016 um 20:29 hat Eric Blake geschrieben:
> On 02/02/2016 10:28 AM, Programmingkid wrote:
> 
> >> Whats the rationale here ? Using pre-allocated fixed
> >> length arrays is pretty bad practice in general, but
> >> especially so for filenames
> > 
> > With an automatic variable there is no worry about when to release it. 
> 
> Yeah, but it comes with the downside of having to worry about exhausting
> stack space (there are platforms where MAXPATHLEN is intentionally
> undefined [hello, GNU Hurd]), or with the downside of arbitrary
> limitations.  And at the same time, MAXPATHLEN is usually wasteful -
> allocating 1k or 4k of stack to store what is typically less than 100
> bytes is dumb.  Really, storing file names in fixed length arrays is
> better off to avoid, and just get used to dynamic management.

Just let me add that while it's probably harmless here in .bdrv_open(),
other paths in the block layer which run in a coroutine have a much more
limited stack size and the danger of stack overflows is very real there.

Kevin

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

[Programmingkid]

On Feb 2, 2016, at 11:36 PM, Eric Blake wrote:

> On 02/02/2016 09:21 PM, Programmingkid wrote:
> 
>>>> #if defined(__APPLE__) && defined(__MACH__)
>>>>       /* if a physical device experienced an error while being opened */
>>>>       if (strncmp((*bsd_path ? bsd_path : filename), "/dev/", 5) == 0) {
>>>>           print_unmounting_directions(*bsd_path ? bsd_path : filename);
>>>>           return -1;
>>>>       }
>>> 
>>> A bit repetitive. You don't use filename after the fact, so shorter
>>> would be:
>>> 
>>> #if defined(__APPLE__)...
>>>   if (*bsd_path) {
>>>       filename = filename;
> 
> Oops, I shouldn't be writing emails late at night.  Let's try this again.
> 
> if (*bsd_path) {
>    filename = bsd_path;
> }
> if (strncmp(filename, ...
> 
> Hopefully that makes more sense.

So you want this:

#if defined(__APPLE__) && defined(__MACH__)
        if (*bsd_path) {
            filename = bsd_path;
        }
        /* if a physical device experienced an error while being opened */
        if (strncmp(filename, "/dev/", 5) == 0) {
            print_unmounting_directions(filename);
            return -1;
        }
#endif /* defined(__APPLE__) && defined(__MACH__) */

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

[Eric Blake]

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

On 02/03/2016 08:54 AM, Programmingkid wrote:
>> Oops, I shouldn't be writing emails late at night.  Let's try this again.

> 
> So you want this:
> 
> #if defined(__APPLE__) && defined(__MACH__)
>         if (*bsd_path) {
>             filename = bsd_path;
>         }
>         /* if a physical device experienced an error while being opened */
>         if (strncmp(filename, "/dev/", 5) == 0) {
>             print_unmounting_directions(filename);
>             return -1;
>         }
> #endif /* defined(__APPLE__) && defined(__MACH__) */

Yes, that should work.

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