Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism

[Sanidhya Kashyap <sanidhya.iiith@gmail.com>]

>> +{ 'command': 'test-vmstates',
>> +  'data': {'*iterations': 'int',
>> +           '*period':     'int',
>> +           'noqdev':      'bool',
> 
> Do we really care about "noqdev", or should we just "decree" that it is
> "false" always?
> 

Okay. Will remove it.

> 
>> +#define DEBUG_TEST_VMSTATES 1
>> +
>> +#ifndef DEBUG_TEST_VMSTATES
>> +#define DEBUG_TEST_VMSTATES 0
>> +#endif
> 
> If you have the previe line, this ones are not needed.
> 
> 
>> +
>> +#define DPRINTF(fmt, ...) \
>> +    do { \
>> +        if (DEBUG_TEST_VMSTATES) { \
>> +            printf("vmstate_test: " fmt, ## __VA_ARGS__); \
>> +        } \
>> +    } while (0)
> 
> DPRINTF is *so* last year O:-)
> It is considedered better to just add tracepoints to the code.  I think
> that all the DPRINTF's make sense to be a tracepoint, no?
> 
>

yup, tracepoints do make sense. Will incorporate that.

> We need a better preffix that test_vmstates_
> But I can't think of one right now.  Will think later about it.
> 
> 

I am really bad with naming conventions :-/. Whatever seems fit to you.
I'll use that.

>> +static inline bool check_device_name(VMStateLogState *v,
>> +                                     VMStatesQdevDevices *qdevices,
>> +                                     Error **errp)
> 
> Is "inline" needed?  I would expect the compiler to do a reasonable
> decision with an static function called only once?
> 

My mistake. Will correct it.

>> +{
>> +    VMStatesQdevResetEntry *qre;
>> +    strList *devices_name = qdevices->device;
>> +    QTAILQ_INIT(&v->qdev_list);
>> +    bool device_present;
>> +
>> +    /* now, checking against each one */
>> +    for (; devices_name; devices_name = devices_name->next) {
>> +        device_present = false;
>> +        VMStatesQdevResetEntry *new_qre;
>> +        QTAILQ_FOREACH(qre, &vmstate_reset_handlers, entry) {
>> +            if (!strcmp(qre->device_name, devices_name->value)) {
>> +
>> +                device_present = true;
>> +
>> +                new_qre = g_malloc0(sizeof(VMStatesQdevResetEntry));
>> +                new_qre->dev = qre->dev;
>> +                strcpy(new_qre->device_name, qre->device_name);
>> +                QTAILQ_INSERT_TAIL(&v->qdev_list, new_qre, entry);
>> +
>> +                break;
>                    return;
> 
> And remove the whole "device_present" variable and assignation?
> 

Actually, I have used this variable which will help us in deciding
whether the user has provided a sane list of vmstates name's or not. If
the names do not match, then we do not proceed. That is why I have used
the device_present variable. I'll change the variable name.

>> +    if (v->noqdev) {
>> +        DPRINTF("resetting all devices\n");
>> +        qemu_system_reset(VMRESET_SILENT);
> 
> What is diffe9rent between calling with "noqdev" and with an empty
> device list?  I would expect them to be the same list of devices.
> 

Well, they are not. Currently, when qemu_system_reset() is called the
mc->reset is NULL. So, the old way of resetting the device takes place
which has some different devices or might be bus registered. Therefore,
I have tried to provide whatever is there on the table i.e related to
the resetting. But, that is not required, I'll completely remove it.

>> +        f = qemu_bufopen("w", NULL);
>> +        if (!f) {
>> +            goto testing_end;
>> +        }
> 
> I think we can call qemu_bufopen() out of the timer, and then doing the
> free on the cleanup?
> 
> 

If I perform the cleanup at the end, then I will be eating the memory.
When I close the buffer, at least that memory is freed. The other issue
will be taking the read pointer back to the write pointer, of which I
don't know whether there is a support or not.

>> +
>> +        cpu_synchronize_all_states();
>> +
>> +        /* saving the vmsates to memory buffer */
>> +        ret = qemu_save_device_state(f);
>> +        if (ret < 0) {
>> +            goto testing_end;
>> +        }
>> +        save_vmstates_duration = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
>> +                                 start_time;
>> +        DPRINTF("iteration: %ld, save time (ms): %ld\n",
>> +                v->current_iteration, save_vmstates_duration);
>> +
>> +        /* clearing the states of the guest */
>> +        test_vmstates_reset_devices(v);
>> +
>> +        start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> +        qsb = qemu_buf_get(f);
>> +        f = qemu_bufopen("r", (QEMUSizedBuffer *)qsb);
> 
> We are only using this function once, can't we convince it to just be
> called "const"?
> 
> 
> ok what are we doing here:
> 
> 
> for(i=0; i< times; i++) {
>        .....
>        f = qemu_bufopen("r", ..);
>        .....
>        f = qemu_buf_get(f);
>        f = qemu_bufopen("w", ..)
>        ...
>        qemu_fclose(f);
> }
> 
> 
> What I propose is switching to something like:
> 
> f = qemu_bufopen("r", ..);
> 
> for(i=0; i< times; i++) {
>        ....
>        qemu_buf_set_ro(f);
>        .....
>        qemu_buf_set_rw(f)
>        ...
> }
> qemu_fclose(f);
> 
> 
> This makes qemu_bufopen() way simpler.  Once there, my understanding is
> that current code is leaking a QEMUBuffer each time that we call
>      qemu_bufopen("w", ...)
> 
> 

Yup, I have missed qemu_fclose(f) before

f = qemu_bufopen("r", ..);

I'll correct it. Now, regarding the qemu_buf_set_ro and qemu_buf_set_rw,
I guess, I'll have to rewind the pointer, for which I have to get some
idea before doing it, or extend the QEMUFile code for memory buffer.


-- 
-----

Sanidhya Kashyap