On 04/17/2013 09:43 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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