11:16 a.m.
On 04/17/2013 09:43 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
Currently the RPC protocol files can contain annotations after
the protocol enum eg
REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, /* autogen autogen
priority:high */
This is not very extensible as the number of annotations grows.
Hmm - wonder if that means you plan on adding annotations soon :)
Change it to use
/**
* @generate: both
* @priority: high
*/
REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247,
At which point grouping by 10 no longer matters.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/locking/lock_protocol.x | 68 +-
src/remote/lxc_protocol.x | 39 +-
src/remote/qemu_protocol.x | 51 +-
src/remote/remote_protocol.x | 1832 +++++++++++++++++++++++++++++++++---------
src/rpc/gendispatch.pl | 67 +-
5 files changed, 1638 insertions(+), 419 deletions(-)
enum virLockSpaceProtocolProcedure {
- /* Each function must have a two-word comment. The first word is
- * whether remote_generator.pl handles daemon, the second whether
- * it handles src/remote. Additional flags can be specified after a
- * pipe.
+ /* Each function must be preceeded by a comment providing one or
s/preceeded/preceded/
+ * more annotations:
+ *
+ * - @generate: none|client|server|both
+ *
+ * Whether to generate the dispatch stubs for the server
+ * and/or client code.
+ *
+ * - @readstream: paramnumber
+ * - @writestream: paramnumber
+ *
+ * The @readstream or @writestream annotations let daemon and src/remote
+ * create a stream. The direction is defined from the src/remote point
+ * of view. A readstream transfers data from daemon to src/remote. The
+ * <paramnumber> specifies at which offset the stream parameter is inserted
+ * in the function parameter list.
+ *
+ * - @priority: low|high
+ *
+ * Each API that might eventually access hypervisor's monitor (and thus
unneeded two spaces
+ * block) MUST fall into low priority. However, there are some
exceptions
+ * to this rule, e.g. domainDestroy. Other APIs MAY be marked as high
and again
+ * priority. If in doubt, it's safe to choose low. Low is
taken as default,
+ * and thus can be left out.
*/
Sounds reasonable.
- VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1, /* skipgen skipgen
*/
- VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2, /* skipgen skipgen */
- VIR_LOCK_SPACE_PROTOCOL_PROC_NEW = 3, /* skipgen skipgen */
- VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_RESOURCE = 4, /* skipgen skipgen */
- VIR_LOCK_SPACE_PROTOCOL_PROC_DELETE_RESOURCE = 5, /* skipgen skipgen */
+ /**
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1,
+ /**
I think it would be nicer to put a blank line before the /** of each
listing, so that it is even more visually obvious that a comment applies
to the enum below it.
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2,
+ /**
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_NEW = 3,
+ /**
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_RESOURCE = 4,
+ /**
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_DELETE_RESOURCE = 5,
- VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6, /* skipgen skipgen */
- VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7, /* skipgen skipgen */
+ /**
Hmm, your conversion preserved existing blank lines, but if you take my
advice of adding a blank before every /**, I don't see any point in
having double blanks before multiples of 5 or 10.
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6,
+ /**
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7,
- VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8 /* skipgen skipgen */
+ /**
+ * @generate: none
+ */
+ VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8
};
Conversion of this file looks fine; I'll assume that the other files
were converted in the same manner without reviewing quite as closely.
+++ b/src/remote/lxc_protocol.x
@@ -37,13 +37,34 @@ const LXC_PROGRAM = 0x00068000;
const LXC_PROTOCOL_VERSION = 1;
enum lxc_procedure {
- /* Each function must have a three-word comment. The first word is
- * whether gendispatch.pl handles daemon, the second whether
- * it handles src/remote.
- * The last argument describes priority of API. There are two accepted
- * values: low, high; Each API that might eventually access hypervisor's
- * monitor (and thus block) MUST fall into low priority. However, there
- * are some exceptions to this rule, e.g. domainDestroy. Other APIs MAY
- * be marked as high priority. If in doubt, it's safe to choose low. */
- LXC_PROC_DOMAIN_OPEN_NAMESPACE = 1 /* skipgen skipgen priority:low */
+ /* Each function must be preceeded by a comment providing one or
Same typo. Looks like you copied the comment around, so copy the typo
fixes around, too.
+ REMOTE_PROC_NUM_OF_DEFINED_NETWORKS = 50,
+
+
+
+ /**
Wow, that really added some whitespace between groups.
+ /**
+ * @generate: both
+ */
+ REMOTE_PROC_DOMAIN_MIGRATE_SET_COMPRESSION_CACHE = 300
Good - no change in the total number of messages.
+
+
/*
* Notice how the entries are grouped in sets of 10 ?
* Nice isn't it. Please keep it this way when adding more.
You can probably drop this comment, if you decide that the extra space
every ten entries doesn't add anything.
+++ b/src/rpc/gendispatch.pl
@@ -82,10 +82,11 @@ sub name_to_TypeName {
# Read the input file (usually remote_protocol.x) and form an
# opinion about the name, args and return type of each RPC.
-my ($name, $ProcName, $id, $flags, %calls, @calls);
+my ($name, $ProcName, $id, $flags, %calls, @calls, %opts);
Conversion looks sane.
ACK with nits addressed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org