From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHEzr-0002Bu-Nb for qemu-devel@nongnu.org; Fri, 30 Jan 2015 12:04:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YHEzo-0008MV-DY for qemu-devel@nongnu.org; Fri, 30 Jan 2015 12:04:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56241) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHEzo-0008ML-5t for qemu-devel@nongnu.org; Fri, 30 Jan 2015 12:04:40 -0500 Message-ID: <54CBB9A5.2070708@redhat.com> Date: Fri, 30 Jan 2015 12:04:37 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <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> In-Reply-To: <20150130143247.GC24537@noname.redhat.com> 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: Kevin Wolf , Markus Armbruster Cc: famz@redhat.com, qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, stefanha@redhat.com 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.' >> 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. > >> 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. >> 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 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. Could be mis-remembering, this whole discussion is spread out over months now.