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

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

[Programmingkid]

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>

---
Fixed some spacing issues. 
Removed else condition in FindEjectableOpticalMedia.
Added continue statement to FindEjectableOpticalMedia.
Replaced printf() with error_report() in FindEjectableOpticalMedia.
Altered comment in FindEjectableOpticalMedia.
If the spacing in this patch looks off, try changing the font to something
that is mono-spaced.

 block/raw-posix.c |  140 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 102 insertions(+), 38 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..9e7de11 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -42,9 +42,9 @@
 #include <IOKit/storage/IOMediaBSDClient.h>
 #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
+#endif /* (__APPLE__) && (__MACH__) */
 
 #ifdef __sun__
 #define _POSIX_PTHREAD_SEMANTICS 1
@@ -1975,32 +1975,46 @@ 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 kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
+                                                                char *mediaType)
 {
     kern_return_t       kernResult;
     mach_port_t     masterPort;
     CFMutableDictionaryRef  classesToMatch;
+    const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
 
     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.\n",
+                                                         matching_array[index]);
+            continue;
+        }
+        CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
+                                                                kCFBooleanTrue);
+        kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
+                                                                 mediaIterator);
+        if (kernResult != KERN_SUCCESS) {
+            error_report("Note: IOServiceGetMatchingServices returned %d\n",
+                                                                    kernResult);
+        }
 
+        /* If a match was found, leave the loop */
+        if (*mediaIterator != 0) {
+            DPRINTF("Matching using %s\n", matching_array[index]);
+            snprintf(mediaType, strlen(matching_array[index])+1, "%s",
+                                                         matching_array[index]);
+            break;
+        }
+    }
     return kernResult;
 }
 
@@ -2033,7 +2047,36 @@ 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, "Error: Failed to find a working partition on "
+                                                                     "disc!\n");
+    } else {
+        DPRINTF("Using %s as optical disc\n", test_partition);
+        pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+    }
+    return partition_found;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
 
 static int hdev_probe_device(const char *filename)
 {
@@ -2115,6 +2158,17 @@ static bool hdev_is_sg(BlockDriverState *bs)
     return false;
 }
 
+/* Prints directions on mounting and unmounting a device */
+static void print_unmounting_directions(const char *file_name)
+{
+    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);
+}
+
 static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
@@ -2125,30 +2179,33 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
 #if defined(__APPLE__) && defined(__MACH__)
     const char *filename = qdict_get_str(options, "filename");
 
-    if (strstart(filename, "/dev/cdrom", NULL)) {
-        kern_return_t kernResult;
+    /* If using a real cdrom */
+    if (strcmp(filename, "/dev/cdrom") == 0) {
+        char bsd_path[MAXPATHLEN];
+        char mediaType[11]; /* IODVDMedia is the longest value */
         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));
+        FindEjectableOpticalMedia(&mediaIterator, mediaType);
+        GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
+        if (mediaIterator) {
+            IOObjectRelease(mediaIterator);
         }
 
-        if ( mediaIterator )
-            IOObjectRelease( mediaIterator );
+        /* 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");
+            return -1;
+        }
+
+        /* If using a cdrom disc and finding a partition on the disc failed */
+        if (strncmp(mediaType, "IOCDMedia", 9) == 0 &&
+                                         setup_cdrom(bsd_path, errp) == false) {
+            print_unmounting_directions(bsd_path);
+            return -1;
+        }
+
+        filename = bsd_path;
+        qdict_put(options, "filename", qstring_from_str(filename));
     }
 #endif
 
@@ -2159,9 +2216,16 @@ 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 && ret != 0) {
+        print_unmounting_directions(filename);
+        return -1;
+    }
+#endif /* defined(__APPLE__) && defined(__MACH__) */
+
     /* Since this does ioctl the device must be already opened */
     bs->sg = hdev_is_sg(bs);
 
-- 
1.7.5.4

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

[Eric Blake]

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

On 11/27/2015 02:49 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>
> 
> ---
> Fixed some spacing issues. 
> Removed else condition in FindEjectableOpticalMedia.
> Added continue statement to FindEjectableOpticalMedia.
> Replaced printf() with error_report() in FindEjectableOpticalMedia.
> Altered comment in FindEjectableOpticalMedia.
> If the spacing in this patch looks off, try changing the font to something
> that is mono-spaced.

Patches are best read in monospaced fonts, anyways; it's better to make
that part of your workflow, and assume that everyone else has already
done likewise, than to advertise that you are only making life harder
for yourself.

> 
>  block/raw-posix.c |  140 ++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 102 insertions(+), 38 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index ccfec1c..9e7de11 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -42,9 +42,9 @@
>  #include <IOKit/storage/IOMediaBSDClient.h>
>  #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
> +#endif /* (__APPLE__) && (__MACH__) */
>  

I have now mentioned in both v8 and v9 that this hunk should be its own
patch (and is simple enough to cc qemu-trivial).  Disregarding reviewers
suggestions is not a good idea - it only serves to waste time (both
yours and reviewers) and earn you black marks, such that it will be even
less likely that anyone wants to review your patches in the first place.
 I'm trying to help you be a better contributor, but it feels like you
are ignoring advice, and so my natural reaction is to ignore you.

>  #ifdef __sun__
>  #define _POSIX_PTHREAD_SEMANTICS 1
> @@ -1975,32 +1975,46 @@ 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 kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
> +                                                                char *mediaType)

No, your indentation is still wrong.  I tried to point out on your v8
that we don't right-justify to 80 columns, but rather left-justify to
the point just after the (.


> +    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.\n",

No. Don't use trailing '.' or trailing '\n' in error_report() calls.
I've already mentioned this, and feel like I'm becoming a broken record.
 When you disregard my review comments, I become disinclined to review
your patches any further.

> +                                                         matching_array[index]);

Indentation is still wrong.

> +            continue;
> +        }
> +        CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
> +                                                                kCFBooleanTrue);
> +        kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> +                                                                 mediaIterator);
> +        if (kernResult != KERN_SUCCESS) {
> +            error_report("Note: IOServiceGetMatchingServices returned %d\n",
> +                                                                    kernResult);

No trailing \n in error_report(), indentation is wrong.

> +        }
>  
> +        /* If a match was found, leave the loop */
> +        if (*mediaIterator != 0) {
> +            DPRINTF("Matching using %s\n", matching_array[index]);
> +            snprintf(mediaType, strlen(matching_array[index])+1, "%s",
> +                                                         matching_array[index]);

Spaces around binary '+', and indentation is wrong.


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

Indentation is wrong.


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

Indentation is wrong, no trailing '!' or '\n' in error_setg().

I'm so bothered by the fact that I already pointed this out in v9 and
you still didn't fix it for v10 that I am not even paying attention to
actual code, and just looking at style violations.  You have effectively
lost me as a valid reviewer on this patch.  I don't like feeling like
this, as I try hard to be welcoming to new contributors, but in the open
source world, you have to return the favor by learning from the advice
you are given, rather than repeating the same mistakes.

-- 
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] [PATCH v10] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Nov 30, 2015, at 11:26 AM, Eric Blake wrote:

> On 11/27/2015 02:49 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>
>> 
>> ---
>> Fixed some spacing issues. 
>> Removed else condition in FindEjectableOpticalMedia.
>> Added continue statement to FindEjectableOpticalMedia.
>> Replaced printf() with error_report() in FindEjectableOpticalMedia.
>> Altered comment in FindEjectableOpticalMedia.
>> If the spacing in this patch looks off, try changing the font to something
>> that is mono-spaced.
> 
> Patches are best read in monospaced fonts, anyways; it's better to make
> that part of your workflow, and assume that everyone else has already
> done likewise, than to advertise that you are only making life harder
> for yourself.
> 
>> 
>> block/raw-posix.c |  140 ++++++++++++++++++++++++++++++++++++++--------------
>> 1 files changed, 102 insertions(+), 38 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ccfec1c..9e7de11 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -42,9 +42,9 @@
>> #include <IOKit/storage/IOMediaBSDClient.h>
>> #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
>> +#endif /* (__APPLE__) && (__MACH__) */
>> 
> 
> I have now mentioned in both v8 and v9 that this hunk should be its own
> patch (and is simple enough to cc qemu-trivial).  Disregarding reviewers
> suggestions is not a good idea - it only serves to waste time (both
> yours and reviewers) and earn you black marks, such that it will be even
> less likely that anyone wants to review your patches in the first place.
> I'm trying to help you be a better contributor, but it feels like you
> are ignoring advice, and so my natural reaction is to ignore you.

I assure you that this change is *required* for my patch. Without it the patch would
not even compile. I need a macro from IODVDMedia.h. If removing the IOCDTypes.h
is what is bothering you, it is a very small change that no one is going to miss. That
header file was commented out but not removed for some reason. 

I do thank you for your patients. I think it might be better if instead of saying "this is wrong",
you talk about what should be done differently more.

> 
>> #ifdef __sun__
>> #define _POSIX_PTHREAD_SEMANTICS 1
>> @@ -1975,32 +1975,46 @@ 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 kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> +                                                                char *mediaType)
> 
> No, your indentation is still wrong.  I tried to point out on your v8
> that we don't right-justify to 80 columns, but rather left-justify to
> the point just after the (.

If you feel it is that important, I will do it. I just thought it was easier to read when your
eye is already in the area. There is less time spend finding the next argument that way.

> 
>> +    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.\n",
> 
> No. Don't use trailing '.' or trailing '\n' in error_report() calls.
> I've already mentioned this, and feel like I'm becoming a broken record.
> When you disregard my review comments, I become disinclined to review
> your patches any further.

I don't remember hearing about not using \n in the error_report() call, but I will
fix this in the next patch.

> 
>> +                                                         matching_array[index]);
> 
> Indentation is still wrong.

Will left justify with the left parenthesis. 

> 
>> +            continue;
>> +        }
>> +        CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>> +                                                                kCFBooleanTrue);
>> +        kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
>> +                                                                 mediaIterator);
>> +        if (kernResult != KERN_SUCCESS) {
>> +            error_report("Note: IOServiceGetMatchingServices returned %d\n",
>> +                                                                    kernResult);
> 
> No trailing \n in error_report(), indentation is wrong.

Ok. 

> 
>> +        }
>> 
>> +        /* If a match was found, leave the loop */
>> +        if (*mediaIterator != 0) {
>> +            DPRINTF("Matching using %s\n", matching_array[index]);
>> +            snprintf(mediaType, strlen(matching_array[index])+1, "%s",
>> +                                                         matching_array[index]);
> 
> Spaces around binary '+', and indentation is wrong.

Ok. Will add spaces and left justify.

> 
> 
>> +    /* 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);
> 
> Indentation is wrong.

Ok. Will left justify also.

> 
> 
>> +    /* 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");
> 
> Indentation is wrong, no trailing '!' or '\n' in error_setg().

Keeping spaces around the messages made them easier to read. But
I'm flexible. Will remove the ! and \n. 

> 
> I'm so bothered by the fact that I already pointed this out in v9 and
> you still didn't fix it for v10 that I am not even paying attention to
> actual code, and just looking at style violations.  You have effectively
> lost me as a valid reviewer on this patch.  I don't like feeling like
> this, as I try hard to be welcoming to new contributors, but in the open
> source world, you have to return the favor by learning from the advice
> you are given, rather than repeating the same mistakes.

Please forgive me of my sins. I am just another human being trying to help
improve QEMU. Practicing a little more patients and tolerance might help
improve your life.

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

[Eric Blake]

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

On 11/30/2015 09:51 AM, Programmingkid wrote:

>>> +++ b/block/raw-posix.c
>>> @@ -42,9 +42,9 @@
>>> #include <IOKit/storage/IOMediaBSDClient.h>
>>> #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
>>> +#endif /* (__APPLE__) && (__MACH__) */
>>>
>>
>> I have now mentioned in both v8 and v9 that this hunk should be its own
>> patch (and is simple enough to cc qemu-trivial).  Disregarding reviewers
>> suggestions is not a good idea - it only serves to waste time (both
>> yours and reviewers) and earn you black marks, such that it will be even
>> less likely that anyone wants to review your patches in the first place.
>> I'm trying to help you be a better contributor, but it feels like you
>> are ignoring advice, and so my natural reaction is to ignore you.
> 
> I assure you that this change is *required* for my patch. Without it the patch would
> not even compile. I need a macro from IODVDMedia.h. If removing the IOCDTypes.h
> is what is bothering you, it is a very small change that no one is going to miss. That
> header file was commented out but not removed for some reason. 

And I assure you that splitting the change into a separate patch, and
making this a 2-patch series, is essential for you getting your patch
applied.  It's okay for one patch to depend on another; it is not okay
to shove multiple fixes into a single patch when you have been told to
split it into multiple patches.

Okay, let me restate things a bit:

The trivial hunk of your patch (that should be applied independently,
because it is unrelated code cleanup) would be:

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

and then the part for _this_ commit (adding a new required header) would be:

> #include <IOKit/storage/IOMediaBSDClient.h>
> #include <IOKit/storage/IOMedia.h>
> #include <IOKit/storage/IOCDMedia.h>
>+#include <IOKit/storage/IODVDMedia.h>
> #include <CoreFoundation/CoreFoundation.h>
> #endif /* (__APPLE__) && (__MACH__) */


>>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>>> +                                                                char *mediaType)
>>
>> No, your indentation is still wrong.  I tried to point out on your v8
>> that we don't right-justify to 80 columns, but rather left-justify to
>> the point just after the (.
> 
> If you feel it is that important, I will do it. I just thought it was easier to read when your
> eye is already in the area. There is less time spend finding the next argument that way.

When you have read THOUSANDS of lines of code indented in one style,
then your eye is already very trained to look in the same place for the
continued line.  One-off code that places the code in somewhere other
than the usual place is actually HARDER to read, because it violates the
conventions that you have already trained to read.

> I don't remember hearing about not using \n in the error_report() call, but I will
> fix this in the next patch.

v8:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05806.html
"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."

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