From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52751) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHGgW-0000HO-31 for qemu-devel@nongnu.org; Fri, 30 Jan 2015 13:52:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YHGgS-0004pV-MO for qemu-devel@nongnu.org; Fri, 30 Jan 2015 13:52:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52771) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHGgS-0004pJ-Fv for qemu-devel@nongnu.org; Fri, 30 Jan 2015 13:52:48 -0500 Date: Fri, 30 Jan 2015 19:52:44 +0100 From: Kevin Wolf Message-ID: <20150130185244.GE24537@noname.redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54CBB9A5.2070708@redhat.com> 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: John Snow Cc: famz@redhat.com, qemu-devel@nongnu.org, Markus Armbruster , Max Reitz , vsementsov@parallels.com, stefanha@redhat.com 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. 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... > >>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. > > > >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. > > > >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. > > > >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. > > > >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. > It makes sense that if we don't keep a "this means node name ONLY" > parameter for any command 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... Kevin