[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command |
Date: |
Mon, 27 Feb 2012 09:26:08 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120216 Thunderbird/10.0.1 |
On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote:
> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <address@hidden> 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
Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command, Kevin Wolf, 2012/02/27
[Qemu-devel] [PATCH v2 2/2] QMP: Add qmp command for blockdev-group-snapshot-sync, Jeff Cody, 2012/02/26