Re: [Qemu-devel] [PATCH V5 4/7] qmp: Allow to change password on named block driver states.

[Fam Zheng <famz@redhat.com>]

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 <benoit@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 <famz@redhat.com>