Re: [Qemu-devel] [PATCH v3 2/5] block: JSON filenames and relative backing files

[Max Reitz <mreitz@redhat.com>]

On 2014-11-25 at 20:57, Eric Blake wrote:
> On 11/24/2014 02:43 AM, Max Reitz wrote:
>> When using a relative backing file name, qemu needs to know the
>> directory of the top image file. For JSON filenames, such a directory
>> cannot be easily determined (e.g. how do you determine the directory of
>> a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow
>> relative filenames for the backing file of BDSs only having a JSON
>> filename.
>>
>> Furthermore, BDS::exact_filename should be used whenever possible. If
>> BDS::filename is not equal to BDS::exact_filename, the former will
>> always be a JSON object.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 27 +++++++++++++++++++++------
>>   block/qapi.c          |  7 ++++++-
>>   include/block/block.h |  5 +++--
>>   3 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0c1be37..a0cddcd 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -305,19 +305,28 @@ void path_combine(char *dest, int dest_size,
>>   
>>   void bdrv_get_full_backing_filename_from_filename(const char *backed,
>>                                                     const char *backing,
>> -                                                  char *dest, size_t sz)
>> +                                                  char *dest, size_t sz,
>> +                                                  Error **errp)
>>   {
>> -    if (backing[0] == '\0' || path_has_protocol(backing)) {
>> +    if (backing[0] == '\0' || path_has_protocol(backing) ||
>> +        path_is_absolute(backing))
>> +    {
> checkpatch.pl didn't complain about this?  The { should be on the
> previous line.  With that fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Oh, actually it does.

But there's no rule about that in CODING_STYLE. There is a rule about { 
being on the same line as the if, though ("The opening brace is on the 
line that contains the control
flow statement that introduces the new block"). With the condition split 
over multiple lines, that is no longer possible; therefore, we can place 
{ anywhere we want.

I always place { on the following line for multi-line conditions because 
I find that more readable[1]; maybe I'll change my mind in a year or 
two, but for now there is no rule about this case and I always do it 
this way; so far, nobody complained. :-)

[1] I don't like the following:

if (foo0() || foo1() || foo2() || foo3() ||
     bar0() || bar1() || bar2() || bar3()) {
     baz();

Because in my eyes it looks like baz() may be part of the condition list 
at first glance (because it is indented just as much as the last line of 
the condition list).

Max