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

[Eric Blake]

On 12/10/2013 08:16 AM, Luiz Capitulino wrote:
> On Tue, 10 Dec 2013 15:25:07 +0100
> Kevin Wolf <address@hidden> wrote:
> 
>> My objection to your approach is strong because Benoît already sent an
>> alternative which I believe is less worse because with it, arguments
>> actually mean what their names tell instead of having additional bools
>> for "oh, and I said A, but I didn't mean it, I really want B".
> 
> Current proposal:
> 
> { 'command': 'block_passwd', 'data': {'*device': 'str',
>                                       '*node-name': 'str', 'password': 'str'} 
> }
> 
> When I look at it, I ask myself:
> 
>  - What happens when device=NULL?

Then, per docs, node-name must be non-NULL.

> 
>  - What happens when node-name=NULL?

Then, per docs, device must be non-NULL.

> 
>  - What happens when device=NULL and node-name=NULL?

Error; violates docs.

> 
>  - What happens when device != NULL and node-node != NULL?

Error; violates docs.

> 
>  - What happens when device != NULL but node-node=NULL?

Operate on the device.

> 
>  - What happens when device=NULL but node-node != NULL?

Operate on the node-name.

> 
> My proposal:
> 
> { 'command': 'block_passwd', 'data': {'device': 'str',
>                                       '*device-is-node': 'bool', 'password': 
> 'str'} }
> 
>  - What happens when device-is-node=NULL?

Operate on the device (same as if device-is-node were false)

> 
>  - What happens when device-is-node != NULL?

The state of the bool determines whether we are operating on device or
on node.

> 
> PS: I'm not NACKing anything. My review to this series started with a
>     "what about" question. I did voice my objections against this
>     proposal, but didn't NACK it. Besides you're a maintainer as much
>     as I am, so I could NACK this as much as you could push this
>     through your tree ignoring review.

And I appreciate the critical review - if we capture design decisions
like this in the commit message, then a year from now when someone asks
"why didn't you do it this alternative way" we can say "look at the
commit message which explores the tradeoffs, and why we settled for what
we did" rather than saying "oops, we painted ourselves into a corner
because we didn't think about that".

The more this goes on, the more I like the mutually-exclusive
device[str]/node-name[str] arguments, and the less I like the
device[str]/device-is-node[bool] solution, but I can still live with
either approach from libvirt's point of view.

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

signature.asc
Description: OpenPGP digital signature