On Mon, Feb 27, 2012 at 2:26 PM, Jeff Cody <jc...@redhat.com> wrote: > On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote: >> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <jc...@redhat.com> 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