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

[Jeff Cody <jcody@redhat.com>]

On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote:
> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <jcody@redhat.com> wrote:
> 
> Do you have automated tests for this feature?
> 

No, not yet.  The testing has been manual.

>> +/*
>> + * 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.

Good point.  How about this:

/*
 * Add new bs contents at the top of an image chain while the chain is
 * live, while keeping required fields on the top layer.
 *
 * This will modify the BlockDriverState fields, and swap contents
 * between bs_new and bs_top.  Both bs_new and bs_top are modified.
 *
 * A new image file is not created.  It is assumed that bs_new already
 * points to an existing image, with the correct backing filename of
 * bs_top->backing_file.
 */


> 
>> + * 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"?

Sorry, that should have been 'bs_top->backing_file'. The image file is
not created by this function.  I added some more explanation, and
corrected that typo, in the above comment block.  Let me know if you
think it still needs more clarification.


> 
>> +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.

OK, thanks. I will add some clarifying comments - you are right, it is a
bit tricky and counter-intuitive.  It may also be clearer if this is
done closer to the actual swap ('*bs_top = tmp').  I will move it down
to above the swap.

> 
>> +
>> +    /* 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!

OK; I will add a blanket warning at the top to inspect bdrv_append()
when adding new fields, to see if they need updated in bdrv_append().

> 
>> +        /* 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.

Previous entries may have been opened. You are right, however, in that
it is not needed at all. We can rely on new_bs being non-NULL.  I will
remove the 'is_open' references.

> 
> 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!)
> 

You are right. That should be bdrv_delete(), and not bdrv_close().  I
will fix that, and also rename close_and_fail to delete_and_fail for
good measure.

Thanks,
Jeff