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

[Kevin Wolf]

Am 10.12.2013 um 16:16 hat Luiz Capitulino geschrieben:
> 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?
> 
>  - What happens when node-name=NULL?
> 
>  - What happens when device=NULL and node-name=NULL?
> 
>  - What happens when device != NULL and node-node != NULL?
> 
>  - What happens when device != NULL but node-node=NULL?
> 
>  - What happens when device=NULL but node-node != NULL?

Looks pretty long, but the latter four are just the combination of the
former two.

I think it's relevant in what context you are looking at it. As a user,
the question I'm really asking is:

    Hm, okay, passing just a password can't be enough, so...
    - ...do I need to specificy device, and if so, with which value?
    - ...do I need to specificy node-name, and if so, with which value?

Looking at the documentation comment would make it clear rather quickly
that I should use only one of them and what the right value is.

For the implementation of the command, I actually need to think about
all the corner cases. However, even there a comment "device and
node-name may never be specified at the same time" makes it pretty clear
what I'm supposed to implement.

For debugging, suppose I read a QMP command in a log file, I read either
"device" or "node-name" and I know exactly what is meant.

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

Okay, as a user, I see that need to pass a device name and a password,
fine. Hm, I really wanted to pass a node name instead... Okay, a second
look shows me the optional flag that changes the meaning of "device", so
I use that, fine.

Implementing the functionality in qemu, this can't be hard. I just need
to check the flag when I search the BlockDriverState so that I search by
node-name or by device name, depending on what is requested. Hopefully
nobody will even use the 'device' parameter outside my if block because
it's overloaded with different meanings, so they would introduce a bug.

Debugging a QMP command in a log file, I see the 'device' key and am
happy because now I know the name of the device that caused trouble. Oh,
there was the "I didn't really mean 'device'" flag set? Whoops...


So perhaps in some cases you have to ask yourself more questions with
the separate fields, but the "whoops" cases of misunderstanding the
overloaded field will mean that people might not even think of asking a
question, but rather just do the wrong thing.

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

I hope we agree that edit wars aren't where we're going to go.

Kevin