Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <jcody@redhat.com> wrote:

Do you have automated tests for this feature?

> +/*
> + * Add new bs contents at the top of an image chain while the chain is live,
> + * while keeping required fields on the top layer.

Please also document the swap behavior.  It's pretty important for the
caller to realize that once this function returns, their
BlockDriverState arguments with have swapped.

> + * It is assumed that bs_new already points to an existing image,
> + * with the correct backing filename of top->backing_file

Not sure what this means.  Isn't bs_new going to use bs_top as its
backing file?  Why "top->backing_file"?

> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    BlockDriverState tmp;
> +
> +    /* the new bs must not be in bdrv_states */
> +    bdrv_make_anon(bs_new);
> +
> +    tmp = *bs_new;
> +    tmp.backing_hd = bs_new;

This is tricky, would be nice to comment that we will swap bs_new and
bs_top later, therefore we need a pointer to bs_new here even though
it doesn't make sense yet.

> +
> +    /* there are some fields that need to stay on the top layer: */
> +
> +    /* dev info */
> +    tmp.dev_ops          = bs_top->dev_ops;
> +    tmp.dev_opaque       = bs_top->dev_opaque;
> +    tmp.dev              = bs_top->dev;
> +    tmp.buffer_alignment = bs_top->buffer_alignment;
> +    tmp.copy_on_read     = bs_top->copy_on_read;
> +
> +    /* i/o timing parameters */
> +    tmp.slice_time        = bs_top->slice_time;
> +    tmp.slice_start       = bs_top->slice_start;
> +    tmp.slice_end         = bs_top->slice_end;
> +    tmp.io_limits         = bs_top->io_limits;
> +    tmp.io_base           = bs_top->io_base;
> +    tmp.throttled_reqs    = bs_top->throttled_reqs;
> +    tmp.block_timer       = bs_top->block_timer;
> +    tmp.io_limits_enabled = bs_top->io_limits_enabled;

Please add a comment into BlockDriverState to warn that changes to
fields may require updates to this function too!

> +        /* We will manually add the backing_hd field to the bs later */
> +        states->new_bs = bdrv_new("");
> +        ret = bdrv_open(states->new_bs, snapshot_file,
> +                        flags | BDRV_O_NO_BACKING, drv);
> +        states->is_open = true;

What is the point of is_open?  If we failed then new_bs is actually
not open here.

I think you can remove this field and just do the following in close_and_fail:

if (states->new_bs) {
    bdrv_delete(states->new_bs);
}

(BTW I think close_and_fail is currently leaking new_bs because it
only closes it and does not delete it!)