[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2] qemu-img: simplify img_convert
From: |
Peter Lieven |
Subject: |
Re: [Qemu-devel] [PATCH V2] qemu-img: simplify img_convert |
Date: |
Fri, 21 Apr 2017 15:54:28 +0200 |
> 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
>