From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50576) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btdOl-00030e-Pu for qemu-devel@nongnu.org; Mon, 10 Oct 2016 12:26:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btdOg-0001W9-7J for qemu-devel@nongnu.org; Mon, 10 Oct 2016 12:25:54 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-18-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: Date: Mon, 10 Oct 2016 18:25:37 +0200 MIME-Version: 1.0 In-Reply-To: <1475232808-4852-18-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="R2NSosSpdOve6VGQRIgp2WJrNBt40sefm" Subject: Re: [Qemu-devel] [PATCH 17/22] qmp: add autoload parameter to block-dirty-bitmap-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --R2NSosSpdOve6VGQRIgp2WJrNBt40sefm From: Max Reitz To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Message-ID: Subject: Re: [PATCH 17/22] qmp: add autoload parameter to block-dirty-bitmap-add References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-18-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1475232808-4852-18-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Optional. Default is false. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Denis V. Lunev > --- > blockdev.c | 22 ++++++++++++++++++++-- > qapi/block-core.json | 7 ++++++- > qmp-commands.hx | 5 ++++- > 3 files changed, 30 insertions(+), 4 deletions(-) Design question: I see that being able to specify these flags when creating bitmaps is useful. However, would about a way for the user to change these flags on an existing dirty bitmap? Would you consider that useful? (Of course, if so, it can always be added later, we don't need it now.) > diff --git a/blockdev.c b/blockdev.c > index ec0ec75..00da7a1 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1992,6 +1992,7 @@ static void block_dirty_bitmap_add_prepare(BlkAct= ionState *common, > qmp_block_dirty_bitmap_add(action->node, action->name, > action->has_granularity, action->granul= arity, > action->has_persistent, action->persist= ent, > + action->has_autoload, action->autoload,= > &local_err); > =20 > if (!local_err) { > @@ -2696,6 +2697,7 @@ out: > void qmp_block_dirty_bitmap_add(const char *node, const char *name, > bool has_granularity, uint32_t granula= rity, > bool has_persistent, bool persistent, > + bool has_autoload, bool autoload, > Error **errp) > { > AioContext *aio_context; > @@ -2729,10 +2731,26 @@ void qmp_block_dirty_bitmap_add(const char *nod= e, const char *name, > if (!has_persistent) { > persistent =3D false; > } > + if (!has_autoload) { > + autoload =3D false; > + } > + > + if (autoload && !persistent) { > + error_setg(errp, "Autoload flag must be used only for persiste= nt" > + "bitmaps"); Missing space between "persistent" and "bitmaps". Also, technically I think you should throw this error if has_autoload is true instead of autoload. I would consider it wrong if a user specified autoload at all, even autoload=3Dfalse, without setting persistent=3Dtrue= =2E But if you disagree, then please keep the condition the way it is. > + goto out; > + } > =20 > bitmap =3D bdrv_create_dirty_bitmap(bs, granularity, name, errp); > - if (bitmap !=3D NULL) { > - bdrv_dirty_bitmap_set_persistance(bitmap, persistent); > + if (bitmap =3D=3D NULL) { > + goto out; > + } > + > + if (persistent) { > + bdrv_dirty_bitmap_set_persistance(bitmap, true); > + if (autoload) { > + bdrv_dirty_bitmap_set_autoload(bitmap, true); > + } By the way, a simpler way to do the same would be just bdrv_dirty_bitmap_set_persistance(bitmap, persistent); bdrv_dirty_bitmap_set_autoload(bitmap, autoload); But if you prefer this explicit style, that's fine, too. > } > =20 > out: > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 2bf56cd..087a681 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1239,11 +1239,16 @@ > # corresponding block device on it's close. Default is fa= lse. > # For block-dirty-bitmap-add. (Since 2.8) > # > +# @autoload: #optional the bitmap will be autoloaded on it's storage i= mage > +# open. I'd rephrase this like "The bitmap will be automatically loaded when the image it is stored in is opened." (Or keep "autoloaded" instead of "automatically loaded", that doesn't really matter.) > This flag is only for persistent bitmap and needed = to inform > +# block driver that bitmap should be autoloaded on the next= image > +# open. Again, the user doesn't really have to care how the internals work. All they need to know is that the image will be automatically loaded when the image is opened the next time, and that is something the first sentence told them already. (Of course, "This flag may only be specified for persistent bitmaps" should stay.) > Default is false. For block-dirty-bitmap-add. (Sinc= e 2.8) Again, I don't see the point of the last sentence. Max > +# > # Since 2.4 > ## > { 'struct': 'BlockDirtyBitmapAdd', > 'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32', > - '*persistent': 'bool' } } > + '*persistent': 'bool', '*autoload': 'bool' } } > =20 > ## > # @block-dirty-bitmap-add > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 434b418..8f4e841 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1441,7 +1441,7 @@ EQMP > =20 > { > .name =3D "block-dirty-bitmap-add", > - .args_type =3D "node:B,name:s,granularity:i?,persistent:b?", > + .args_type =3D "node:B,name:s,granularity:i?,persistent:b?,au= toload:b?", > .mhandler.cmd_new =3D qmp_marshal_block_dirty_bitmap_add, > }, > =20 > @@ -1461,6 +1461,9 @@ Arguments: > - "persistent": bitmap will be saved to corresponding block device > on it's close. Block driver should maintain persistent= bitmaps > (json-bool, optional, default false) (Since 2.8) > +- "autoload": only for persistent bitmaps. Bitmap will be autoloaded o= n it's > + storage image open. (json-bool, optional, default false)= > + (Since 2.8) > =20 > Example: > =20 >=20 --R2NSosSpdOve6VGQRIgp2WJrNBt40sefm Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX+8EBEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQDNT CACc8reBd5IDJTudm76HPZaqicBpSz7+kKRt8GiYOXc+T1qyRP3yePaLxgy19g/1 5fw9zNmMF3UqC4fE6lXRkWtZ5/EXQQ9Hx7CSL9IK8KgM+maAYlITA8Tbu/mvOYAH 0Y+wT1l9eisVIsK6+Wxc9KlI1gkf3e4Z+9QWupy/8toBmRDCoAToU00tSIKwckeo atUaQEHDbk/uoFZyGvIHjrGoMZaQYGwNPtvR8Je7QUruYbzOSmoDlhcT8et/1R4b 1bfvC85NSDlsSsnzqMjsOQp25MWGDfkNLBU0FaeXnS2v9kqeHIsgYgsK75aPC/NM ljXmU5TRtTGT/W+M2AVluyHO =+nAm -----END PGP SIGNATURE----- --R2NSosSpdOve6VGQRIgp2WJrNBt40sefm--