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

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 2:26 PM, Jeff Cody <address@hidden> wrote:
> 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.

For image streaming I used the Python unittest framework along with
QMP/qmp.py to create tests.  I am going to submit it as a qemu-iotest.
 We really need something along the lines of a harness with QMP
support so that these block layer features can be tested.  I will CC
you on the email.

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

Looks good.

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

I still don't follow.  Old bs_top's image file itself becomes the
backing file, not bs_top->backing_file.  Perhaps I'm misinterpreting
because of how swap changes bs_top and bs_new, but I'm reading it from
the perspective of the caller when they pass in bs_top.

The rest looks good.

Stefan