Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

18.05.2020 22:07, Eric Blake wrote:
> On 5/18/20 6:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.05.2020 04:16, Eric Blake wrote:
>>> Include actions for --add, --remove, --clear, --enable, --disable, and
>>> --merge (note that --clear is a bit of fluff, because the same can be
>>> accomplished by removing a bitmap and then adding a new one in its
>>> place, but it matches what QMP commands exist).  Listing is omitted,
>>> because it does not require a bitmap name and because it was already
>>> possible with 'qemu-img info'.  A single command line can play one or
>>> more bitmap commands in sequence on the same bitmap name (although all
>>> added bitmaps share the same granularity, and and all merged bitmaps
>>> come from the same source file).  Merge defaults to other bitmaps in
>>> the primary image, but can also be told to merge bitmaps from a
>>> distinct image.
>>>
>>> While this supports --image-opts for the file being modified, I did
>>> not think it worth the extra complexity to support that for the source
>>> file in a cross-file merges.  Likewise, I chose to have --merge only
>>> take a single source rather than following the QMP support for
>>> multiple merges in one go (although you can still use more than one
>>> --merge in the command line); in part because qemu-img is offline and
>>> therefore atomicity is not an issue.
>>>
> 
>>> +
>>> +    blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
>>> +                   false);
>>
>> fit in one line
> 
> That line would be exactly 80; I tend to wrap at 79 or earlier rather than exactly on 80.
> 
>>
>>> +    if (!blk) {
>>> +        goto out;
>>> +    }
>>> +    bs = blk_bs(blk);
>>> +    if (src_filename) {
>>> +        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
>>> +                       false);
>>
>> s/NULL/false/
> 
> D'oh.  And yet it still compiles.  Fixing.
> 
>>
>> also, fit in one line
> 
> Yes, this one's shorter.  Fixing.
> 
> 
>>> +
>>> +        if (err) {
>>> +            error_reportf_err(err, "Operation %s on bitmap %s failed",
>>
>> s/failed/failed: /
>>
>>> +                              op, bitmap);
>>> +            ret = -1;
>>
>> dead assignment: you never set ret after first initialization to -1.
> 
> Fixing both.
> 
>>
>>> +            goto out;
>>> +        }
>>> +        g_free(act);
>>> +    }
>>> +
>>> +    ret = 0;
>>> +
>>> + out:
>>> +    blk_unref(src);
>>> +    blk_unref(blk);
>>> +    qemu_opts_del(opts);
>>> +    if (ret) {
>>> +        return 1;
>>> +    }
>>> +    return 0;
>>
>> Hmm, as it's the only usage of ret, you may initialize it to 1 at function start, and here just "return ret;"
> 
> Yep, done.
> 
>>> +DEF("bitmap", img_bitmap,
>>> +    "bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b source_file [-F source_fmt]] [-g granularity] [--object objectdef] [--image-opts | -f fmt] filename bitmap")
>>> +SRST
>>> +.. option:: bitmap (--merge SOURCE | --add | --remove | --clear | --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME BITMAP
>>
>> Not about this patch, but it's a pity that we have triple duplications (triplications ?) of such lines..
> 
> Yes, it is annoying.  But as you say, it's a cleanup for another day, for someone who is interested.
> 
>>
>> With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny suggestions):
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Thanks; squashing in:
> 
> diff --git i/qemu-img.c w/qemu-img.c
> index 8c99e68ba8aa..f940137cb0e5 100644
> --- i/qemu-img.c
> +++ w/qemu-img.c
> @@ -4493,7 +4493,7 @@ typedef struct ImgBitmapAction {
>   static int img_bitmap(int argc, char **argv)
>   {
>       Error *err = NULL;
> -    int c, ret = -1;
> +    int c, ret = 1;
>       QemuOpts *opts = NULL;
>       const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
>       const char *filename, *bitmap;
> @@ -4641,8 +4641,7 @@ static int img_bitmap(int argc, char **argv)
>       }
>       bs = blk_bs(blk);
>       if (src_filename) {
> -        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> -                       false);
> +        src = img_open(false, src_filename, src_fmt, 0, false, false, false);
>           if (!src) {
>               goto out;
>           }
> @@ -4695,9 +4694,8 @@ static int img_bitmap(int argc, char **argv)
>           }
> 
>           if (err) {
> -            error_reportf_err(err, "Operation %s on bitmap %s failed",
> +            error_reportf_err(err, "Operation %s on bitmap %s failed: ",
>                                 op, bitmap);
> -            ret = -1;
>               goto out;
>           }
>           g_free(act);
> @@ -4709,10 +4707,7 @@ static int img_bitmap(int argc, char **argv)
>       blk_unref(src);
>       blk_unref(blk);
>       qemu_opts_del(opts);
> -    if (ret) {
> -        return 1;
> -    }
> -    return 0;
> +    return ret;
>   }
> 
>   #define C_BS      01
> 

Great, keep my r-b with this.

-- 
Best regards,
Vladimir