Re: [Qemu-devel] [Qemu-block] [PATCH v1 1/1] qcow2 resize with snapshots

[Eric Blake]

On 05/19/2016 01:46 AM, zhangzhiming wrote:

First, a disclaimer: thanks for writing a patch, and for trying to
improve the code.  The rest of this mail may sound negative, but take it
as advice for how to make your efforts more likely to succeed.

In the subject line, it looks like you manually added "[Qemu-block]",
but did not actually CC: address@hidden  Don't manually add
that, the list does it on your behalf if you actually send to the
intended lists (any patch mail that goes to qemu-block should also be
sent to qemu-devel).

> hi, i wrote some code for 'qcow2 resize' with snapshot with v3 image and 
> 'qcow2 goto’ too, 
> different size of snapshots are supported. 
> and i have tested the function and it  seems work well.
> there are some code copied from snapshot_delete_blkdev_internal, and 
> qmp_block_resize, 
> it feels not very good.
> 
> please review it for me. thanks.

It's implied that any patch sent to the list will be reviewed.  And this
particular sentence doesn't add any value once the patch is applied to
git.  It might be better after the '---' separator (information to the
reviewer that this is one of your first submissions, and needs extra
care in the review) rather than as part of the commit message proper, if
it is needed at all.

> 
> zhangzhiming
> address@hidden

Unfortunately, your patch is missing a Signed-off-by, which is an
absolute must before it can be accepted.  Also, I noticed you sent
several followups; it's better to polish your patch series and send a v2
with all the pieces in one coherent set (perhaps split across multiple
patches) than it is to make reviewers string together multiple emails
into a single patch.

At this point, I'm just doing a high-level stylistic review, not even
caring about content or design, since you have a number of things that
need fixing before the patch can be considered ready.

More tips on proper patch submission:

http://wiki.qemu.org/Contribute/SubmitAPatch

> 
> --
> 
> diff --git a/block.c b/block.c

Your patch is missing a diffstat, which is a very handy tool in giving a
quick summary of what you touch below.

> index 18a497f..047698a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2632,6 +2632,24 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>  }
>  
>  /**
> + *  goto a snapshot

Not the most informative of comments, and redundant with the function
name. In this case, the function name is self-descriptive enough that
you might be able to get away without a comment, although a comment
probably is better.

> + */
> +int bdrv_apply_snapshot(BlockDriverState *bs, const char *snapshot_id, 
> uint64_t snapshot_size)

Long line. ./scripts/checkpatch.pl should have flagged it as needing
line wrapping.

> +{
> +    int ret = bdrv_snapshot_goto(bs, snapshot_id);
> +    if(ret < 0){

Coding style violations (again, checkpatch is your friend).  Space after
'if', and before '{'.

Since this function can fail, it should probably take an Error **errp
parameter to pass a detailed message about the failure back to the
caller, rather than just an error code with no further information.

> +        return ret;
> +    }
> +
> +    ret = refresh_total_sectors(bs, snapshot_size);
> +    bdrv_dirty_bitmap_truncate(bs);

Is it safe to call this even if ret indicates that
refresh_total_sectors() failed?

> +    if (bs->blk) {
> +        blk_dev_resize_cb(bs->blk);

Can bdrv_dirty_bitmap_truncate() or blk_dev_resize_cb() fail? If so,
what error recovery should you take?

> +++ b/block/qcow2.c
> @@ -2501,15 +2501,17 @@ static int qcow2_truncate(BlockDriverState *bs, 
> int64_t offset)
>          return -EINVAL;
>      }
>  
> -    /* cannot proceed if image has snapshots */
> -    if (s->nb_snapshots) {
> -        error_report("Can't resize an image which has snapshots");
> +    bool v3_truncate = (s->qcow_version == 3);
> +
> +    /* cannot proceed if image has snapshots and qcow_version is not 3*/

Space before */

> +    if (!v3_truncate && s->nb_snapshots) {
> +        error_report("Can't resize an image which has snapshots and 
> qcow_version is not 3");

Long line

>          return -ENOTSUP;
>      }
>  
> -    /* shrinking is currently not supported */
> -    if (offset < bs->total_sectors * 512) {
> -        error_report("qcow2 doesn't support shrinking images yet");
> +    /* shrinking is supported from version 3*/
> +    if (!v3_truncate && offset < bs->total_sectors * 512) {
> +        error_report("qcow2 doesn't support shrinking images yet while 
> qcow_version is not 3");

And again

> +++ b/blockdev.c
> @@ -2961,6 +2961,120 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> +SnapshotInfo *qmp_blockdev_snapshot_goto_internal_sync(const char *device,
> +                             bool has_id,

Alignment is off.


> +
> +    if(!has_id){

Multiple style problems throughout the function.

> +++ b/hmp-commands.hx
> @@ -1159,6 +1159,24 @@ Delete an internal snapshot on device if it support
>  ETEXI
>  
>      {
> +        .name       = "snapshot_goto_blkdev_internal",
> +        .args_type  = "device:B,name:s,id:s?",
> +        .params     = "device name [id]",
> +        .help       = "apply an internal snapshot of device.\n\t\t\t"
> +                      "If id is specified, qemu will try apply\n\t\t\t"
> +                      "the snapshot matching both id and name.\n\t\t\t"
> +                      "The format of the image used by device must\n\t\t\t"
> +                      "support it, such as qcow2.\n\t\t\t",
> +        .mhandler.cmd = hmp_snapshot_goto_blkdev_internal,

Why do we need a new command?  Especially one with the name 'internal'
in it?  Why are the existing commands not extensible to call the new
behavior?

> +++ b/pixman
> @@ -1 +1 @@
> -Subproject commit 87eea99e443b389c978cf37efc52788bf03a0ee0
> +Subproject commit 7c6066b700c7cdd4aeb8be426b14b3a5f0de4b6c

Oops, that should not be part of your commit.

Also, adding a new HMP command but no counterpart QMP command is fishy.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature