Re: [Qemu-devel] [PATCH V2] qemu-img: simplify img_convert

[Peter Lieven]

> Am 21.04.2017 um 15:43 schrieb Eric Blake <address@hidden>:
> 
>> On 04/21/2017 04:11 AM, Peter Lieven wrote:
>> img_convert has been around before there was an ImgConvertState or
>> a block backend, but it has never been modified to directly use
>> these structs. Change this by parsing parameters directly into
>> the ImgConvertState and directly use BlockBackend where possible.
>> Furthermore variable initialization has been reworked and sorted.
>> 
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>> V1->V2: - fix s.target_has_backing [Fam]
>>        - remove compress and use s.compressed everywhere [Kevin]
>>        - Fix indent at one place
>>        - Fix typos in commit msg [Eric]
>>        - Further organize the local stack variables
>> 
> 
>> -    int c, bs_n, bs_i, compress, cluster_sectors, skip_create;
>> -    int64_t ret = 0;
>> -    int progress = 0, flags, src_flags;
>> -    bool writethrough, src_writethrough;
>> -    const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, 
>> *out_filename;
>> +    int c, bs_i, flags, src_flags = 0;
>> +    const char *fmt = NULL, *out_fmt = "raw", *cache = "unsafe",
>> +               *src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL,
>> +               *out_filename, *out_baseimg_param, *snapshot_name = NULL;
> 
> I'm not necessarily a fan of cramming as many variables as possible
> under one type declaration. It creates more churn (aka larger diffs)
> down the road if one of the variables has to change type, compared to
> the case where every variable has its own line.  But nothing in HACKING
> requires either crammed-together or one-per-line declarations, so it's
> not worth changing.
> 
>> 
>> 
>> -    if (bs_n > 1 && out_baseimg) {
>> +    if (s.src_num > 1 && out_baseimg) {
>>         error_report("-B makes no sense when concatenating multiple input "
>>                      "images");
> 
> 
> There's other patches on list that will conflict in this region.  But I
> think the plan is that Kevin will take your v2, then have those patches
> rebased on top of yours.
> 
>> @@ -2224,9 +2204,10 @@ static int img_convert(int argc, char **argv)
>>     if (out_baseimg_param) {
>>         out_baseimg = out_baseimg_param;
>>     }
>> +    s.target_has_backing = (bool) out_baseimg;
> 
> That's an unusual spelling. The compiler does the right thing without
> the cast, and if you are still worried, writing !!out_baseimg is less
> typing (and fewer casts) for the same effect.
> 

The cast is exactly the same as before this patch. I thought there was a reason 
for it.
Kevin, can you change it when applying?

Thanks,
Peter

> At any rate, it fixes the failures I'm seeing on iotests 019.
> 
> Reviewed-by: Eric Blake <address@hidden>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>