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

[Jeff Cody]

On 02/27/2012 09:40 AM, Stefan Hajnoczi wrote:
> 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.

Excellent, thanks.

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

Maybe it would be better to just replace that part of the comment with
something that says "This function does not create the image file".

The function bdrv_append() will neither create the image file, or set
(in the case of qcow2) the extended headers of the file to have the
backing filename.  It is only concerned with the bs contents.

For example, in group snapshots in blockdev.c, the image file is first
created, and then we do the swap:

        /* create new image w/backing file */
        ret = bdrv_img_create(snapshot_file, format,
                              states->old_bs->filename,
                              drv->format_name, NULL, -1, flags);

        ...

        ret = bdrv_open(states->new_bs, snapshot_file,
                        flags | BDRV_O_NO_BACKING, drv);

        ...

        bdrv_append(states->new_bs, states->old_bs);




> The rest looks good.
> 
> Stefan