Re: [Qemu-devel] [PATCH 01/12] migration: Introduce announce parameters

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3387 bytes --]

On 05/24/2017 01:05 PM, Vladislav Yasevich wrote:
> Add parameters that control RARP/GARP announcement timeouts.
> The parameters structure is added to the QAPI and a qmp command
> is added to set/get the parameter data.
> 
> Based on work by "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---

Just an interface review for now:

> +++ b/qapi-schema.json
> @@ -569,6 +569,90 @@
>  ##
>  { 'command': 'query-events', 'returns': ['EventInfo'] }
>  
> +
> +##
> +# @AnnounceParameter:
> +#
> +# @AnnounceParameter enumeration
> +#
> +# @initial: Initial delay (in ms) before sending the first GARP/RARP
> +#       announcement
> +#
> +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets

s/announcemnt/announcement/

> +#
> +# @rounds: Number of self-announcement attempts
> +#
> +# @step: Delay increate (in ms) after each self-announcment attempt

s/increate/increase/
s/announcment/announcement/

> +#
> +# Since: 2.10
> +##
> +{ 'enum' : 'AnnounceParameter',
> +  'data' : [ 'initial', 'max', 'rounds', 'step' ] }

Why are we creating an enum here?  Without reading further, it looks
like you plan on using the enum to delineate members of a union? But
that feels like it will be overly complicated.  A struct should be
sufficient (each parameter being an optional member of the struct, where
you can supply as many or as few on input, but all are reported on output).

> +
> +##
> +# @AnnounceParameters:
> +#
> +# Optional members may be omited on input, but all values will be present

s/omited/omitted/

> +# on output.
> +#           
> +# @initial: Initial delay (in ms) before sending the first GARP/RARP
> +#       announcement
> +#
> +# @max: Maximum delay (in ms) to between GARP/RARP announcemnt packets

s/announcemnt/announcement/

> +#
> +# @rounds: Number of self-announcement attempts
> +#
> +# @step: Delay increate (in ms) after each self-announcment attempt

s/increate/increase/
s/announcment/announcement/

> +#
> +# Since: 2.10
> +##
> +
> +{ 'struct': 'AnnounceParameters',
> +  'data': { '*initial': 'int',
> +            '*max': 'int',
> +            '*rounds': 'int',
> +            '*step': 'int' } }
> +
> +##
> +# @announce-set-parameters:
> +#
> +# Set qemu announce parameters.
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute": "announce-set-parameters",
> +#      "arguments": { "announce-rounds": 10 } }
> +#
> +##
> +{ 'command': 'announce-set-parameters', 'boxed': true,
> +  'data': 'AnnounceParameters' }
> +
> +##
> +# @query-announce-parameters:
> +#
> +# Returns information about the current announce parameters
> +#
> +# Returns: @AnnounceParameters
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute": "query-announce-parameters" }
> +# <- { "return": {
> +#          "initial": 50,
> +#          "max": 500,
> +#          "rounds": 5,
> +#          "step": 100
> +#       }
> +#    }
> +#
> +##
> +{ 'command': 'query-announce-parameters',
> +  'returns': 'AnnounceParameters' }

Yep, I'm right. The enum is bogus.  The struct is sufficient, so you
don't need the enum.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]