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

[Kevin Wolf]

Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben:
> 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 <address@hidden>
> ---
>  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)

This is a normal function without generated callers, so can we get rid
of has_device/has_node_name and just switch to passing NULL if it's not
present? That would make it more convenient to use this function in
other places.

Kevin