[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Switch to a more extensible annotation system for RPC protocols



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]