On Thu, 12/12 16:33, Benoît Canet wrote:
> There was two candidate ways to implement named node manipulation:
>
> 1)
> { 'command': 'block_passwd', 'data': {'*device': 'str',
> '*node-name': 'str', 'password': 'str'}
> }
>
> 2)
>
> { 'command': 'block_passwd', 'data': {'device': 'str',
> '*device-is-node': 'bool',
> 'password': 'str'} }
>
> Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to
> rewrite the QMP block interface for 2.0.
>
> 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.
>
> Kevin argumented that 2 what a clear abuse of the device field and would 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.
>
> Kevin argued that all bs are node including devices ones so 2 does not make
> sense.
>
> Kevin also argued that rewriting the QMP block interface would not make
> disapear
> the current one.
>
> Kevin pushed the argument that making the QAPI generator compatible with the
> semantic of the operation would need a rewrite that no one has done yet.
>
> A vote has been done on the list to elect the version to use and 1 won.
>
> For reference the complete thread is:
> "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block
> driver
> states."
>
> Signed-off-by: Benoit Canet <ben...@irqsave.net>
> ---
> 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(-)
>
> 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;
> }
>
> +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + Error **errp)
> +{
> + BlockDriverState *bs = NULL;
> +
> + if (has_device == has_node_name) {
> + error_setg(errp, "Use either device or node-name but not both");
> + return NULL;
> + }
> +
> + if (has_device) {
> + bs = bdrv_find(device);
> +
> + if (!bs) {
> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + return NULL;
> + }
> +
> + return bs;
> + }
> +
> + bs = 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_force,
> bool force, Error **errp)
> eject_device(bs, force, errp);
> }
>
> -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 = NULL;
> BlockDriverState *bs;
> int err;
>
> - bs = bdrv_find(device);
> - if (!bs) {
> - error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + bs = 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;
> }
>
> 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 *qdict)
> const char *password = qdict_get_str(qdict, "password");
> Error *errp = NULL;
>
> - qmp_block_passwd(device, password, &errp);
> + qmp_block_passwd(true, device, false, NULL, password, &errp);
> hmp_handle_error(mon, &errp);
> }
>
> 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 command,
> 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
> password 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': 'str'} }
> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> + '*node-name': 'str', 'password':
> 'str'} }
>
> ##
> # @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
>
> {
> .name = "block_passwd",
> - .args_type = "device:B,password:s",
> + .args_type = "device:s?,node-name:s?,password:s",
> .mhandler.cmd_new = qmp_marshal_input_block_passwd,
> },
>
> @@ -1465,6 +1465,7 @@ Set the password of encrypted block devices.
> Arguments:
>
> - "device": device name (json-string)
> +- "node-name": name in the block driver state graph (json-string)
> - "password": password (json-string)
>
> Example:
> --
> 1.8.3.2
>
>
Reviewed-by: Fam Zheng <f...@redhat.com>
