From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S21Wv-0003cx-Rv for qemu-devel@nongnu.org; Mon, 27 Feb 2012 09:26:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S21Wn-0005rN-Lx for qemu-devel@nongnu.org; Mon, 27 Feb 2012 09:26:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:3806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S21Wn-0005rE-DW for qemu-devel@nongnu.org; Mon, 27 Feb 2012 09:26:13 -0500 Message-ID: <4F4B9280.20807@redhat.com> Date: Mon, 27 Feb 2012 09:26:08 -0500 From: Jeff Cody MIME-Version: 1.0 References: 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 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. >> +/* >> + * 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