Re: [Qemu-devel] [PATCH 3/4] block/parallels: split check for parallels format in parallels_open

["Denis V. Lunev" <den@parallels.com>]

On 24/07/14 22:50, Jeff Cody wrote:
> On Tue, Jul 22, 2014 at 05:19:36PM +0400, Denis V. Lunev wrote:
>> and rework error path a bit. There is no difference at the moment, but
>> the code will be definitely shorter when additional processing will
>> be required for WithouFreSpacExt
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 8f9ec8a..02739cf 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -85,12 +85,10 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto fail;
>>       }
>>   
>> -    if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
>> -        (le32_to_cpu(ph.version) != HEADER_VERSION)) {
>> -        error_setg(errp, "Image not in Parallels format");
>> -        ret = -EINVAL;
>> -        goto fail;
>> -    }
>> +    if (le32_to_cpu(ph.version) != HEADER_VERSION)
>> +        goto fail_format;
>> +    if (memcmp(ph.magic, HEADER_MAGIC, 16))
>> +        goto fail_format;
> QEMU coding style dictates these statements have curly braces, even
> though they are just one liners.  (If you run your patches against
> scripts/checkpatch.pl, it should catch most style issues).
ok, I just used a kernel convention. Will redo this here and in the
next path. This is not a problem :) Thank you for pointing this
out.

> I think this patch could also just be squashed into patch 4, if
> desired.
I'd prefer not to do this to have behavior changing and behavior
non-changing stuff separated.

>>   
>>       bs->total_sectors = (uint32_t)le64_to_cpu(ph.nb_sectors);
>>   
>> @@ -120,6 +118,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>       qemu_co_mutex_init(&s->lock);
>>       return 0;
>>   
>> +fail_format:
>> +    error_setg(errp, "Image not in Parallels format");
>> +    ret = -EINVAL;
>>   fail:
>>       g_free(s->catalog_bitmap);
>>       return ret;
>> -- 
>> 1.9.1
>>
>>