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

[Programmingkid]

On Nov 30, 2015, at 11:26 AM, Kevin Wolf wrote:

> Am 30.11.2015 um 17:19 hat Eric Blake geschrieben:
>> 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.
> 
> Nor leading "Error:", for that matter.

I just think that using "Error" does communicate the fact that something is 
wrong
a lot better than just printing the message.