Re: [Qemu-devel] [PATCH v10] 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: 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 --]