From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40082) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VqPeE-0004b5-HE for qemu-devel@nongnu.org; Tue, 10 Dec 2013 10:55:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VqPe8-0006QV-4K for qemu-devel@nongnu.org; Tue, 10 Dec 2013 10:54:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16948) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VqPe7-0006QB-MZ for qemu-devel@nongnu.org; Tue, 10 Dec 2013 10:54:52 -0500 Date: Tue, 10 Dec 2013 16:54:49 +0100 From: Kevin Wolf Message-ID: <20131210155449.GH3656@dhcp-200-207.str.redhat.com> References: <1386263703-19292-5-git-send-email-benoit@irqsave.net> <20131206092703.5d60345a@redhat.com> <52A1EC31.7000709@redhat.com> <20131206115215.0427a956@redhat.com> <20131209162309.GJ3549@dhcp-200-207.str.redhat.com> <20131209114109.1c4a8d5f@redhat.com> <20131210095750.GC3656@dhcp-200-207.str.redhat.com> <20131210090620.4bc73895@redhat.com> <20131210142507.GE3656@dhcp-200-207.str.redhat.com> <20131210101613.7a38dadc@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20131210101613.7a38dadc@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: famz@redhat.com, =?iso-8859-1?Q?Beno=EEt?= Canet , jcody@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Am 10.12.2013 um 16:16 hat Luiz Capitulino geschrieben: > On Tue, 10 Dec 2013 15:25:07 +0100 > Kevin Wolf wrote: >=20 > > My objection to your approach is strong because Beno=EEt already sent= an > > alternative which I believe is less worse because with it, arguments > > actually mean what their names tell instead of having additional bool= s > > for "oh, and I said A, but I didn't mean it, I really want B". >=20 > Current proposal: >=20 > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': = 'str'} } >=20 > When I look at it, I ask myself: >=20 > - What happens when device=3DNULL? >=20 > - What happens when node-name=3DNULL? >=20 > - What happens when device=3DNULL and node-name=3DNULL? >=20 > - What happens when device !=3D NULL and node-node !=3D NULL? >=20 > - What happens when device !=3D NULL but node-node=3DNULL? >=20 > - What happens when device=3DNULL but node-node !=3D 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: >=20 > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', 'passw= ord': 'str'} } > > - What happens when device-is-node=3DNULL? >=20 > - What happens when device-is-node !=3D 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