From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35287) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGaKq-0000oH-53 for qemu-devel@nongnu.org; Tue, 25 Sep 2012 14:58:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TGaKp-0005KF-1R for qemu-devel@nongnu.org; Tue, 25 Sep 2012 14:58:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33202) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TGaKo-0005K9-Om for qemu-devel@nongnu.org; Tue, 25 Sep 2012 14:58:18 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8PIwHsv014957 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 25 Sep 2012 14:58:17 -0400 Message-ID: <5061FEC7.7060709@redhat.com> Date: Tue, 25 Sep 2012 14:58:15 -0400 From: Jeff Cody MIME-Version: 1.0 References: <1efc2c8c9b7c215b787507afa5a34c0175f7f4f0.1348589526.git.jcody@redhat.com> <5061F41F.4080502@redhat.com> In-Reply-To: <5061F41F.4080502@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/7] block: add live block commit functionality Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org On 09/25/2012 02:12 PM, Eric Blake wrote: > On 09/25/2012 10:29 AM, Jeff Cody wrote: >> This adds the live commit coroutine. This iteration focuses on the >> commit only below the active layer, and not the active layer itself. >> >> The behaviour is similar to block streaming; the sectors are walked >> through, and anything that exists above 'base' is committed back down >> into base. At the end, intermediate images are deleted, and the >> chain stitched together. Images are restored to their original open >> flags upon completion. >> >> Signed-off-by: Jeff Cody > >> +void commit_start(BlockDriverState *bs, BlockDriverState *base, >> + BlockDriverState *top, int64_t speed, >> + BlockErrorAction on_error, BlockDriverCompletionFunc *cb, >> + void *opaque, Error **errp) >> +{ >> + CommitBlockJob *s; >> + BlockReopenQueue *reopen_queue = NULL; >> + int orig_overlay_flags; >> + int orig_base_flags; >> + BlockDriverState *overlay_bs; >> + Error *local_err = NULL; >> + >> + if ((on_error == BLOCK_ERR_STOP_ANY || >> + on_error == BLOCK_ERR_STOP_ENOSPC) && >> + !bdrv_iostatus_is_enabled(bs)) { >> + error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); >> + return; >> + } >> + >> + overlay_bs = bdrv_find_overlay(bs, top); >> + >> + if (overlay_bs == NULL) { >> + error_setg(errp, "Could not find overlay image for %s:", top->filename); >> + return; >> + } >> + >> + orig_base_flags = bdrv_get_flags(base); >> + orig_overlay_flags = bdrv_get_flags(overlay_bs); > > I think you are missing a check here that base is on the backing chain > of top. See also my comments to 5/7. > Did you mean your comments on 6/7 (or am I missing an email)? This does get partially validated in patch 5/7, in the qmp_block_commit() handler - both base and top are verified to be in the chain 'bs'. What is not validated, however, is that you did not swap your 'top' and 'base' arguments. I'll add a check here for that, to make sure that base is reachable from overlay_bs. >> + >> + /* convert base_bs & overlay_bs to r/w, if necessary */ >> + if (!(orig_base_flags & BDRV_O_RDWR)) { >> + reopen_queue = bdrv_reopen_queue(reopen_queue, base, >> + orig_base_flags | BDRV_O_RDWR); >> + } >> + if (!(orig_overlay_flags & BDRV_O_RDWR)) { >> + reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, >> + orig_overlay_flags | BDRV_O_RDWR); >> + } >> + if (reopen_queue) { >> + bdrv_reopen_multiple(reopen_queue, &local_err); > > Is it valid to make a no-op call, such as: > > { "execute":"block-commit", "arguments":{ > "device":"drive0", "top":"base", "base":"base" }} > > If so, should we do an early exit here, rather than temporarily changing > base to R/W just to change it back to R/O? > > If not, should we be rejecting it up front (again, back to the question > of failing if 'base' is not a backing file of 'top', even if both 'top' > and 'base' are backing files of 'device'). > I'll add a quick check for that as well. Patch 5 checks for it in one scenario (default 'top'), and returns a generic error of: "Invalid files for merge: top and base are the same" I'll make sure to check for that in all scenarios (or just make the test mentioned earlier incorporate top == base as well) Thanks, Jeff