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

[Eric Blake]

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

signature.asc
Description: OpenPGP digital signature