Re: [Qemu-devel] [RFC V3 4/7] qmp: Allow block_passwd to manipulate bs graph nodes.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]

On 12/03/2013 06:26 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  
> +BlockDriverState * bdrv_lookup_bs(bool has_device, const char * device,
> +                                  bool has_node_name, const char * node_name,

Style: no space after * (3 instances)

> +                                  Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    if ((has_device && has_node_name) ||
> +        (!has_device && !has_node_name)) {

Could be shortened to:

if (has_device == has_node_name) {

> +        error_setg(errp, "Use either device or node-name but not both.");

We tend to avoid trailing '.' on error messages

>  
> -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)

Again, no space after '*'

> +++ b/include/block/block.h
> @@ -371,6 +371,9 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag);
>  const char *bdrv_get_format_name(BlockDriverState *bs);
>  BlockDriverState *bdrv_find(const char *name);
>  BlockDriverState *bdrv_find_node(const char *node_name);
> +BlockDriverState * bdrv_lookup_bs(bool has_device, const char * device,
> +                                  bool has_node_name, const char * node_name,
> +                                  Error **errp);

And again

> +++ b/qapi-schema.json
> @@ -1675,7 +1675,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 1.8)

2.0

>  #
>  # @password: the password to use for the device
>  #
> @@ -1689,7 +1693,8 @@
>  #
>  # Since: 0.14.0
>  ##
> -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} }
> +{ 'command': 'block_passwd', 'data': {'*device': 'str',
> +                                      '*node-name': 'str', 'password': 'str'} }

Seems like a reasonable addition; shouldn't cause any back-compat
problems (older management tools will always provide the now-optional
'device').

Is it intentional that you are not exposing this new functionality in HMP?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]