From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49693) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIOjo-0001Xv-BN for qemu-devel@nongnu.org; Mon, 02 Feb 2015 16:40:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIOjk-0003S2-VO for qemu-devel@nongnu.org; Mon, 02 Feb 2015 16:40:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41206) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIOjk-0003Ru-NZ for qemu-devel@nongnu.org; Mon, 02 Feb 2015 16:40:52 -0500 Message-ID: <54CFEEE0.9090004@redhat.com> Date: Mon, 02 Feb 2015 16:40:48 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-4-git-send-email-jsnow@redhat.com> <54B93014.8010203@redhat.com> <54B940F7.7000904@redhat.com> <87oapvnkv1.fsf@blackfin.pond.sub.org> <54BD7191.2060404@redhat.com> <87sif57t8k.fsf@blackfin.pond.sub.org> <54BE86F0.2000408@redhat.com> <878ugwpjdv.fsf@blackfin.pond.sub.org> <20150130143247.GC24537@noname.redhat.com> <54CBB9A5.2070708@redhat.com> <20150130185244.GE24537@noname.redhat.com> <87twz4hber.fsf@blackfin.pond.sub.org> In-Reply-To: <87twz4hber.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Kevin Wolf Cc: vsementsov@parallels.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, Max Reitz On 02/02/2015 05:10 AM, Markus Armbruster wrote: > Kevin Wolf writes: > >> Am 30.01.2015 um 18:04 hat John Snow geschrieben: >>> >>> >>> On 01/30/2015 09:32 AM, Kevin Wolf wrote: >>>> Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben: >>>>> I'm afraid I forgot much of the discussion we had before the break, and >>>>> only now it's coming back, slowly. >>>>> >>>>> Quoting myself on naming parameters identifying nodes[*]: >>>>> >>>>> John Snow pointed out to me that we still haven't spelled out how this >>>>> single parameter should be named. >>>>> >>>>> On obvious option is calling it node-name, or FOO-node-name when we have >>>>> several. However, we'd then have two subtly different kinds of >>>>> parameters called like that: the old ones accept *only* node names, the >>>>> new ones also accept backend names, which automatically resolve to the >>>>> backend's root node. >>>>> >>>>> Three ways to cope with that: >>>>> >>>>> * Find a better name. >>>>> >>>>> * Make the old ones accept backend names, too. Only a few, not that >>>>> much work. However, there are exceptions: >>>>> >>>>> - blockdev-add's node-name *defines* the node name. >>>>> >>>>> - query-named-block-nodes's node-name *is* the node's name. >>>>> >>>>> * Stop worrying and embrace the inconsistency. The affected commands >>>>> are headed for deprecation anyway. >>>>> >>>>> I think I'd go with "node" or "FOO-node" for parameters that reference >>>>> nodes and accept both node names and backend names, and refrain from >>>>> touching the existing node-name parameters. >>>> >>>> Wasn't the conclusion last time that we would try to find a better name >>>> for new commands and leave old commands alone because they are going to >>>> become deprecated? That is, a combination of your first and last option? >>>> >>> >>> That was my impression, too: Use a new name for new commands and >>> then slowly phase out the old things. This makes the new name clear >>> as to what it supports (BOTH backends and nodes through a common >>> namespace) to external management utilities like libvirt. >>> >>> That's why I just rolled 'node-ref.' >> >> Yes, I agree with that in principle. I called it 'node' below because >> that's shorter and doesn't include type information in the name, but if >> everyone preferred 'node-ref', I wouldn't have a problem with it either. > > > >>>>> Let's go through existing uses of @node-name again: >>>>> >>>>> 1. Define a node name >>>>> >>>>> QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror >>>>> >>>>> 2. Report a node name >>>>> >>>>> QMP command query-named-block-nodes (type BlockDeviceInfo) >>>> >>>> Whatever name we end up using, 1. and 2. should probably use the same. >>> >>> Should they? If these commands accept directly *node* names and have >>> no chance of referencing a backend, maybe they should use different >>> parameter names. > > Maybe. But even if we use different names for things that can only be > node names, never backend names, 1. and 2. should use the same name, > because they're both things that can only be node names. That's what > Kevin said. > >> Note that these cases don't refer to a node, but they define/return the >> name of a node. No way that could ever be a backend name, unless we >> broke the code. >> >>>>> 3. Node reference with backend names permitted for convenience >>>>> >>>>> New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and >>>>> others >>>>> >>>>> 4. Node reference with backend names not permitted >>>>> >>>>> QMP commands drive-mirror @replaces, change-backing-file >>>>> @image-node-name >>>>> >>>>> We may want to support the "backend name resolves to root node" >>>>> convenience feature here, for consistency. Then this moves under 3. >>>>> >>>>> Note interface wart: change-backing-file additionally requires the >>>>> backend owning the node. We need the backend to set op-blockers, we >>>>> can't easily find it from the node, so we make the user point it out >>>>> to us. >>>> >>>> These shouldn't be existing. As you say, we should move them to 3. >>>> >>> >>> Technically #3 here isn't a usage of "node-name," because... I >>> didn't use node-name for these commands. Unless I am reading this >>> list wrong, but it's definitely not an "existing use." >>> >>> I don't have any opinions about #4; presumably that's something >>> we're aiming to phase out. >> >> Yes. Where "phasing out" simply means to extend it so that it accepts >> backend names. That should in theory be an easy change, except that >> @image-node-name has a name that isn't quite compatible with it... > > Our choice for 3. affects the phasing out of 4. > > Our choice for 3 is a naming convention for parameters referencing nodes > that accept both node and backend names. > > If, after extending the code to accept backend names, the old names for > 4. follow the naming convention for 3., we're done. > > Else, we still have an interface wart. We can live with it, or we can > rename the parameter to follow the convention. > >>>>> 5. "Pair of names" node reference, specify exactly one >>>>> >>>>> QMP commands block_passwd, block_resize, blockdev-snapshot-sync >>>>> >>>>> We can ignore this one, because we intend to replace the commands and >>>>> deprecate the old ones. >>>> >>>> Agreed, these shouldn't be existing either. >>>> >>>>> If I understand you correctly, you're proposing to use @node-name or >>>>> @FOO-node-name when the value must be a node name (items 1+2 and 4), and >>>>> @node-ref or @FOO-node-ref where we additionally support the "backend >>>>> name resolves to root node" convenience feature (item 3). >>>>> >>>>> Is that a fair description of your proposal? >>>>> >>>>> PRO: the name makes it clear when the convenience feature is supported. >>>>> >>>>> CON: if we eliminate 4 by supporting the convenience feature, we either >>>>> create ugly exceptions to the naming convention, or rename the >>>>> parameters. >>>>> >>>>> Opinions? >>>> >>>> If we don't have any cases where node names are allowed, but backend >>>> names are not, then there is no reason to have two different names. I've >>>> yet to see a reason for having commands that can accept node names, but >>>> not backend names. > > Cases 1. and 2. But I'm not sure using different names there to > emphasize "backend names not possible here" would be useful. > >>>> It's a bit different when the command can already accept both, but uses >>>> two separate arguments for it. But I think most of them will be >>>> deprecated, so we can ignore them here. > > Yes, case 5. > >>>> As for the naming, I'm not that sure that it's even useful to add >>>> something to the field name. After all, this is really the _type_ of the >>>> object, not the name. We don't have fields like 'read-only-bool' either. > > Yes, but there the type is actually bool, which makes the bool-ness > perfectly obvious. > > Here, the type is string. That's why I feel a FOO-node convention could > be useful. > >>>> If we're more specifically looking at things that actually refer to >>>> block devices, you already mentioned drive-mirrors @replaces, which is a >>>> great name in my opinion. @replaces-node-ref wouldn't improve anything. >>>> Likewise, blockdev-add already refers to 'file' and 'backing' instead of >>>> 'file-node' or 'backing-node-ref'. >>>> >>>> This probably means that FOO-node-{ref,name} shouldn't exist, because >>>> just FOO is as good or better. The question is a bit harder where there >>>> is only one node involved and we don't have a nice word to describe its >>>> role for the command. This is where we used to use 'device' in the past, >>>> when node-level addressing didn't exist yet. I think just 'node' would >>>> be fine there. > > I'm fine with just 'node'. I like it better than 'node-ref' or > 'node-name'. > > What sets the 'node-name' convention apart is that the existing > (FOO-)node-name already fit this convention. A quick grep finds nine > occurences, in block-core.json and event.json. Advantage if we want to > keep them, disadvantage if we want to get rid of them. > >>>> >>>> Kevin >>>> >>> >>> I'd be happy with naming things "node" (or node-ref, either is fine) >>> going forward; and leaving the old commands (node-name) alone. I >>> seem to recall there was a reason we didn't want to just keep using >>> node-name for the new unified parameters. >> >> It's probably a bad name when we accept backend names as well. And I'm >> not completely sure, but I think we considered removing the relative >> recent node-name again, which has to be probed by management tools >> anyway. We can't remove 'device', which has always been there, and >> keeping both as optional fields isn't really nice. > > Sounds like we want to get rid of them. > >>> It makes sense that if we don't keep a "this means node name ONLY" >>> parameter for any command > > Cases 1. and 2. > >>> then there is no reason to make some >>> distinction between that and "this parameter accepts both," but I >>> think for purposes of libvirt, it is helpful to have a concrete >>> distinction between versions that it can rely on. >> >> Well, at least for new commands that doesn't matter. >> >>> Could be mis-remembering, this whole discussion is spread out over >>> months now. >> >> Yes, remembering all the details is hard... > > Quite a hairball :) > I can roll v12 tomorrow using parameters named "node" if that sounds agreeable to everyone, fixing up the other issues noted by other reviewers in the process. Sound good? --js