From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44873) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5S0S-0007l9-BQ for qemu-devel@nongnu.org; Mon, 20 Jan 2014 22:28:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W5S0N-0003y3-AL for qemu-devel@nongnu.org; Mon, 20 Jan 2014 22:28:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:3249) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5S0N-0003xy-1X for qemu-devel@nongnu.org; Mon, 20 Jan 2014 22:27:59 -0500 Date: Tue, 21 Jan 2014 11:27:55 +0800 From: Fam Zheng Message-ID: <20140121032755.GG29842@T430.redhat.com> References: <1386862440-8003-1-git-send-email-benoit@irqsave.net> <1386862440-8003-5-git-send-email-benoit@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1386862440-8003-5-git-send-email-benoit@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com On Thu, 12/12 16:33, Beno=EEt Canet wrote: > There was two candidate ways to implement named node manipulation: >=20 > 1) > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': = 'str'} > } >=20 > 2) >=20 > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', > 'password': 'str'} } >=20 > Luiz proposed 1 and says 2 was an abuse of the QMP interface and propos= ed to > rewrite the QMP block interface for 2.0. >=20 > Luiz does not like in 1 the fact that 2 fields are optional but one of = them must > be specified leading to an abuse of the QMP semantic. >=20 > Kevin argumented that 2 what a clear abuse of the device field and woul= d not be > practical when reading fast some log file because the user would read "= device" > and think that a device is manipulated when it's in fact a node name. > Documentation of 1 make it pretty clear what to do for the user. >=20 > Kevin argued that all bs are node including devices ones so 2 does not = make > sense. >=20 > Kevin also argued that rewriting the QMP block interface would not make= disapear > the current one. >=20 > Kevin pushed the argument that making the QAPI generator compatible wit= h the > semantic of the operation would need a rewrite that no one has done yet. >=20 > A vote has been done on the list to elect the version to use and 1 won. >=20 > For reference the complete thread is: > "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names blo= ck driver > states." >=20 > Signed-off-by: Benoit Canet > --- > block.c | 32 ++++++++++++++++++++++++++++++++ > blockdev.c | 13 +++++++++---- > hmp.c | 2 +- > include/block/block.h | 3 +++ > qapi-schema.json | 9 +++++++-- > qmp-commands.hx | 3 ++- > 6 files changed, 54 insertions(+), 8 deletions(-) >=20 > diff --git a/block.c b/block.c > index 78d13e5..22190a4 100644 > --- a/block.c > +++ b/block.c > @@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void) > return list; > } > =20 > +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, > + bool has_node_name, const char *node_= name, > + Error **errp) > +{ > + BlockDriverState *bs =3D NULL; > + > + if (has_device =3D=3D has_node_name) { > + error_setg(errp, "Use either device or node-name but not both"= ); > + return NULL; > + } > + > + if (has_device) { > + bs =3D bdrv_find(device); > + > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return NULL; > + } > + > + return bs; > + } > + > + bs =3D bdrv_find_node(node_name); > + > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, node_name); > + return NULL; > + } > + > + return bs; > +} > + > BlockDriverState *bdrv_next(BlockDriverState *bs) > { > if (!bs) { > diff --git a/blockdev.c b/blockdev.c > index 204ab40..838df50 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_for= ce, bool force, Error **errp) > eject_device(bs, force, errp); > } > =20 > -void qmp_block_passwd(const char *device, const char *password, Error = **errp) > +void qmp_block_passwd(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + const char *password, Error **errp) > { > + Error *local_err =3D NULL; > BlockDriverState *bs; > int err; > =20 > - bs =3D bdrv_find(device); > - if (!bs) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + bs =3D bdrv_lookup_bs(has_device, device, > + has_node_name, node_name, > + &local_err); > + if (error_is_set(&local_err)) { > + error_propagate(errp, local_err); > return; > } > =20 > diff --git a/hmp.c b/hmp.c > index 32ee285..3820fbe 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qd= ict) > const char *password =3D qdict_get_str(qdict, "password"); > Error *errp =3D NULL; > =20 > - qmp_block_passwd(device, password, &errp); > + qmp_block_passwd(true, device, false, NULL, password, &errp); > hmp_handle_error(mon, &errp); > } > =20 > diff --git a/include/block/block.h b/include/block/block.h > index 8c10123..f7d8017 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -376,6 +376,9 @@ const char *bdrv_get_format_name(BlockDriverState *= bs); > BlockDriverState *bdrv_find(const char *name); > BlockDriverState *bdrv_find_node(const char *node_name); > BlockDeviceInfoList *bdrv_named_nodes_list(void); > +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, > + bool has_node_name, const char *node_= name, > + Error **errp); > BlockDriverState *bdrv_next(BlockDriverState *bs); > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), > void *opaque); > diff --git a/qapi-schema.json b/qapi-schema.json > index 0dadd5d..903fcb6 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1676,7 +1676,11 @@ > # determine which ones are encrypted, set the passwords with this comm= and, and > # then start the guest with the @cont command. > # > -# @device: the name of the device to set the password on > +# Either @device or @node-name must be set but not both. > +# > +# @device: #optional the name of the block backend device to set the p= assword on > +# > +# @node-name: #optional graph node name to set the password on (Since = 2.0) > # > # @password: the password to use for the device > # > @@ -1690,7 +1694,8 @@ > # > # Since: 0.14.0 > ## > -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'st= r'} } > +{ 'command': 'block_passwd', 'data': {'*device': 'str', > + '*node-name': 'str', 'password':= 'str'} } > =20 > ## > # @balloon: > diff --git a/qmp-commands.hx b/qmp-commands.hx > index d644fe9..1451c1a 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1452,7 +1452,7 @@ EQMP > =20 > { > .name =3D "block_passwd", > - .args_type =3D "device:B,password:s", > + .args_type =3D "device:s?,node-name:s?,password:s", > .mhandler.cmd_new =3D qmp_marshal_input_block_passwd, > }, > =20 > @@ -1465,6 +1465,7 @@ Set the password of encrypted block devices. > Arguments: > =20 > - "device": device name (json-string) > +- "node-name": name in the block driver state graph (json-string) > - "password": password (json-string) > =20 > Example: > --=20 > 1.8.3.2 >=20 >=20 Reviewed-by: Fam Zheng