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

[Eric Blake]

On 11/27/2015 12:35 PM, Programmingkid wrote:

>> Unusual indentation; more typical is:
>>
>> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
>> *mediaIterator,
>> | char *mediatType)
> 
> I agree. I wanted the second long to be right justified with the 80 character 
> line count.

No.  We don't right-justify code to 80 columns.  That's not how it is
done.  Trying to do it just makes you look like the proverbial 'kid' in
your pseudonym, rather than an adult to be taken seriously.

Really, PLEASE follow the indentation patterns of the rest of the code
base - where continued lines are left-justified to be underneath the
character after (, and NOT right-justified to 80 columns.  Violating
style doesn't make your code invalid, but does make your patches less
likely to be applied.


>>> +        /* If you found a match, leave the loop */
>>> +        if (*mediaIterator != 0) {
>>> +            DPRINTF("Matching using %s\n", matching_array[index]);
>>> +            snprintf(mediaType, strlen(matching_array[index])+1, "%s",
>>
>> Spaces around binary '+'.
> 
> What's wrong with no spaces around the plus sign?

Again, the prevailing conventions in the code base is that you put
spaces around every binary operator.  Yes, there is existing old code
that does not meet the conventions, but it is not an excuse to add new
code that is gratuitously different.

> 
>>
>>> +    /* 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");
>>
>> and I already pointed out on v8 that this is not the correct usage of
>> error_setg().  So, here's hoping v10 addresses the comments here and
>> elsewhere.
> 
> Kevin Wolf wanted it this way. What would you do instead?

Keven and I both want you to use error_setg(), but to use it correctly -
and the correct way is to NOT supply a trailing \n.

> 
> Thank you very much for reviewing my patches.

The least you can do for showing that gratitude is to actually improve
your next revisions along the lines of the comments you have been given.
 Quit making it feel like pulling teeth just to get your patches to
match the coding conventions prevalent in the project.

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

signature.asc
Description: OpenPGP digital signature