On 04/17/2013 09:43 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list