Re: [Qemu-devel] [RFC PATCH v3 3/6] VMstate test: basic VMState testing mechanism

[Eric Blake <eblake@redhat.com>]

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

On 08/09/2014 12:26 AM, Sanidhya Kashyap wrote:
> This patch implements the basic way of testing the VMStates' information
> whether it is correct or not while saving and loading the states. The qmp
> interface - test-vmstates can take three parameters as an input to test
> the device states. Now, one can check for any of the devices that have been
> registered with the SaveVMHandlers aka the migration protocol. Similarly,
> the hmp interface (test_vmstates) has only two input parameters - iterations and
> period.
> 

This paragraph:

vvvvvvv
> I have removed the following from the patch on previous comments:
> * replaed DPRINTF with trace_##name.
> * removed the noqdev and completely removed the support for resetting of devices
> based on qemu_system_reset()
^^^^^^^

should not be part of the commit message proper, but...

> 
> As Juan pointed out that there might be a memory leak as I did not close the
> QEMUFile pointer. But, it is not required as that pointer is directly referenced
> by the QEMUBuffer that has been implemented by David. So, IMHO the case pointed
> out by Juan does not exist.
> 
> 
> Signed-off-by: Sanidhya Kashyap <sanidhya.iiith@gmail.com>
> ---

...listed here, as a changelog to guide reviewers.  Remember, anything
before the --- should stand alone as what you would read in qemu.git,
without regards to how many revisions the patch went through; and
anything after the --- is useful for reviewers but intentionally
stripped by the maintainer when using 'git am' as fluff that doesn't
help explain the commit in isolation.

>  hmp-commands.hx  |  16 ++++
>  hmp.c            |  17 ++++
>  hmp.h            |   1 +
>  qapi-schema.json |  22 ++++++
>  qmp-commands.hx  |  33 ++++++++
>  savevm.c         | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events     |   2 +
>  7 files changed, 324 insertions(+)
> 

> +++ b/qapi-schema.json
> @@ -3506,3 +3506,25 @@
>  ##
>  { 'command': 'query-devices',
>    'returns': [ 'QemuDevice' ] }
> +
> +##
> +# @test-vmstates
> +#
> +# tests the vmstates' value by dumping and loading in memory
> +#
> +# @iterations: (optional) The total iterations for vmstate testing.

For consistency, and in case we ever start generating documentation from
the .json file,
s/(optional)/#optional/

> +# The min and max defind value is 10 and 100 respectively.

s/defind value is/defined values are/

> +#
> +# @period: (optional) sleep interval between iteration (in milliseconds).

s/(optional)/#optional/

> +# The default interval is 100 milliseconds with min and max being
> +# 1 and 10000 respectively.
> +#
> +# @devices: (optional) helps in resetting particular device(s) that
> +# have been registered with SaveStateEntry.
> +#
> +# Since 2.2
> +##
> +{ 'command': 'test-vmstates',
> +  'data': {'*iterations':  'int',
> +           '*period':      'int',
> +           '*devices':   [ 'QemuDevice' ] } }

> +Example:
> +
> +-> { "execute": "test-vmstates",
> +     "arguments": {
> +        "iterations": 10,
> +        "period": 100, } }

That is not valid JSON.  You cannot have a trailing , inside {}.  Also,
it might be worth an example of the proper use of the optional "devices"
parameter.


> +
> +static inline bool test_vmstates_check_device_name(VMStateLogState *v,

The compiler is good enough about inlining static functions that you
seldom need to use 'inline'.  That, and 'static inline' has changed
semantics over the years of gcc, so you really want to avoid it unless
you know what you are doing.


> +static void test_vmstates_cb(void *opaque)
> +{
> +    VMStateLogState *v = opaque;
> +    int saved_vm_running = runstate_is_running();
> +    const QEMUSizedBuffer *qsb;

> +
> +        qsb = qemu_buf_get(f);
> +
> +        /* clearing the states of the guest */
> +        test_vmstates_reset_devices(v);
> +
> +        start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +        f = qemu_bufopen("r", (QEMUSizedBuffer *)qsb);

Ewww.  Why are you casting away const?  Make qsb the correct type to
begin with.

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


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