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

[Eric Blake]

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.

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

signature.asc
Description: OpenPGP digital signature