From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37120) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S22QF-0002jz-8U for qemu-devel@nongnu.org; Mon, 27 Feb 2012 10:23:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S22Q6-0007PL-JO for qemu-devel@nongnu.org; Mon, 27 Feb 2012 10:23:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:24930) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S22Q6-0007PH-BK for qemu-devel@nongnu.org; Mon, 27 Feb 2012 10:23:22 -0500 Message-ID: <4F4B9FE5.4000807@redhat.com> Date: Mon, 27 Feb 2012 10:23:17 -0500 From: Jeff Cody MIME-Version: 1.0 References: <4F4B9280.20807@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, lcapitulino@redhat.com, pbonzini@redhat.com, eblake@redhat.com On 02/27/2012 09:40 AM, Stefan Hajnoczi wrote: > On Mon, Feb 27, 2012 at 2:26 PM, Jeff Cody wrote: >> On 02/27/2012 06:13 AM, Stefan Hajnoczi wrote: >>> On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody 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