Re: [Qemu-devel] [RFC PATCH v1 4/5] VMState test: set the frequency of the vmstate testing process

[Eric Blake]

On 07/07/2014 11:18 AM, Sanidhya Kashyap wrote:
> This patch introduces the mechanism to update the sleep interval - sinterval.
> 
> Signed-off-by: Sanidhya Kashyap <address@hidden>
> ---
>  hmp-commands.hx  | 15 +++++++++++++++
>  hmp.c            | 14 ++++++++++++++
>  hmp.h            |  1 +
>  qapi-schema.json | 10 ++++++++++
>  qmp-commands.hx  | 22 ++++++++++++++++++++++
>  savevm.c         | 13 +++++++++++++
>  6 files changed, 75 insertions(+)
> 

> +++ b/qapi-schema.json
> @@ -3492,3 +3492,13 @@
>  { 'command': 'test-vmstates',
>    'data': {'*times'     : 'int',
>             '*sinterval' : 'int' } }
> +
> +## @test-vmstates-set-sinterval
> +#
> +# sets the sleep interval between iterations of the vmstate testing process
> +# @sinterval: the updated sleep interval value

in what units?

> +#
> +# Since 2.1

2.2.

> +##
> +{ 'command' : 'test-vmstates-set-sinterval',
> +  'data'    : { 'sinterval': 'int' } }

This feels like a write-only command.  How do I query what it is
currently set to?

I'm also afraid that we aren't learning our lessons from migrate.
Adding one new command per new tunable does not scale very well in the
number of commands that need to be maintained or that have to be wired
up in management software; better is adding a single command pair for
set/get that can be easily introspected (to see when new tunables are
added), by taking an array of tunables where each tunable is a struct
containing name, type, and value information.  Something like:

{"execute":"test-vmstates-set-tuning", "arguments":{ "list":[
  { "name":"sinterval", "type":"int", "value":100 },
  { "name":"other", "type":"...", "value":... }
]}}
{"return": {}}

and a counterpart:

{"execute":"test-vmstates-get-tuning" }
{"return": [
  { "name":"sinterval", "type":"int", "value":100 },
  { "name":"other", "type":"...", "value":... }
]}


> +void qmp_test_vmstates_set_sinterval(int64_t sinterval, Error **errp)
> +{
> +    VMStateLogState *v = vmstate_current_state();
> +    if (sinterval < TEST_VMSTATE_MIN_INTERVAL_MS ||
> +        sinterval > TEST_VMSTATE_MAX_INTERVAL_MS) {
> +        error_setg(errp, "sleep interval value must be "
> +                   "in the defined range [%d, %d](ms)\n",
> +                   TEST_VMSTATE_MIN_INTERVAL_MS, 
> TEST_VMSTATE_MAX_INTERVAL_MS);

This range limit should also be mentioned in the public API (.json) file.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature