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

[Benoît Canet]

Le Wednesday 04 Dec 2013 à 16:56:05 (-0700), Eric Blake a écrit :
> On 12/03/2013 06:26 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <address@hidden>
> > ---
> >  
> > +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?

Yes, I don't foresee any way to print the graph in HMP so I am limiting the
changes to QMP.

Best regards

Benoît

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