From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53388) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDJ68-0000io-Aa for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:06:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XDJ62-0005xG-4c for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:06:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10050) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDJ61-0005x2-SO for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:06:34 -0400 Message-ID: <53DBF341.40201@redhat.com> Date: Fri, 01 Aug 2014 22:06:25 +0200 From: Max Reitz MIME-Version: 1.0 References: <1406402531-9278-1-git-send-email-mreitz@redhat.com> <1406402531-9278-2-git-send-email-mreitz@redhat.com> <20140731075103.GE707@irqsave.net> In-Reply-To: <20140731075103.GE707@irqsave.net> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH alt 1/7] block: Add status callback to bdrv_amend_options() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 31.07.2014 09:51, Beno=EEt Canet wrote: > The Saturday 26 Jul 2014 =E0 21:22:05 (+0200), Max Reitz wrote : > >> Depending on the changed options and the image format, >> bdrv_amend_options() may take a significant amount of time. In these >> cases, a way to be informed about the operation's status is desirable. >> >> Since the operation is rather complex and may fundamentally change the >> image, implementing it as AIO or a couroutine does not seem feasible. = On >> the other hand, implementing it as a block job would be significantly >> more difficult than a simple callback and would not add benefits other >> than progress report to the amending operation, because it should not >> actually be run as a block job at all. >> >> A callback may not be very pretty, but it's very easy to implement and >> perfectly fits its purpose here. >> >> Signed-off-by: Max Reitz >> --- >> block.c | 5 +++-- >> block/qcow2.c | 5 ++++- >> include/block/block.h | 5 ++++- >> include/block/block_int.h | 3 ++- >> qemu-img.c | 2 +- >> 5 files changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/block.c b/block.c >> index 3e252a2..4c8d4d8 100644 >> --- a/block.c >> +++ b/block.c >> @@ -5708,12 +5708,13 @@ void bdrv_add_before_write_notifier(BlockDrive= rState *bs, >> notifier_with_return_list_add(&bs->before_write_notifiers, notif= ier); >> } >> =20 >> -int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts) >> +int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts, >> + BlockDriverAmendStatusCB *status_cb) >> { >> if (!bs->drv->bdrv_amend_options) { >> return -ENOTSUP; >> } >> - return bs->drv->bdrv_amend_options(bs, opts); >> + return bs->drv->bdrv_amend_options(bs, opts, status_cb); >> } >> =20 >> /* This function will be called by the bdrv_recurse_is_first_non_fil= ter method >> diff --git a/block/qcow2.c b/block/qcow2.c >> index b0faa69..757f890 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2210,7 +2210,8 @@ static int qcow2_downgrade(BlockDriverState *bs,= int target_version) >> return 0; >> } >> =20 >> -static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts) >> +static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, >> + BlockDriverAmendStatusCB *status_cb) >> { >> BDRVQcowState *s =3D bs->opaque; >> int old_version =3D s->qcow_version, new_version =3D old_version= ; >> @@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState = *bs, QemuOpts *opts) >> int ret; >> QemuOptDesc *desc =3D opts->list->desc; >> =20 >> + (void)status_cb; > This look like a temporary hack to please the compiler. > Am I right ? Should be comment this ? It's removed in one of the next patches anyway; but I'll add a comment=20 in v2. Max >> + >> while (desc && desc->name) { >> if (!qemu_opt_find(opts, desc->name)) { >> /* only change explicitly defined options */ >> diff --git a/include/block/block.h b/include/block/block.h >> index 32d3676..f2b1799 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -309,7 +309,10 @@ typedef enum { >> =20 >> int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheck= Mode fix); >> =20 >> -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts); >> +typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t o= ffset, >> + int64_t total_work_size); >> +int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts, >> + BlockDriverAmendStatusCB *status_cb); >> =20 >> /* external snapshots */ >> bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index f6c3bef..5c5798d 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -228,7 +228,8 @@ struct BlockDriver { >> int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result, >> BdrvCheckMode fix); >> =20 >> - int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts); >> + int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts, >> + BlockDriverAmendStatusCB *status_cb); >> =20 >> void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent eve= nt); >> =20 >> diff --git a/qemu-img.c b/qemu-img.c >> index ef74ae4..90d6b79 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -2827,7 +2827,7 @@ static int img_amend(int argc, char **argv) >> goto out; >> } >> =20 >> - ret =3D bdrv_amend_options(bs, opts); >> + ret =3D bdrv_amend_options(bs, opts, NULL); >> if (ret < 0) { >> error_report("Error while amending options: %s", strerror(-r= et)); >> goto out; >> --=20 >> 2.0.3 >> >>