[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testin
From: |
Sanidhya Kashyap |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism |
Date: |
Tue, 29 Jul 2014 23:29:12 +0530 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 |
>> +{ '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
- Re: [Qemu-devel] [PATCH RFC v2 02/12] reset handler for qdevified devices, (continued)
- [Qemu-devel] [PATCH RFC v2 03/12] VMState test: query command to extract the qdevified device names, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 04/12] VMState test: hmp interface for showing qdevified devices, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 05/12] VMstate test: basic VMState testing mechanism, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 06/12] VMState test: hmp interface for vmstate testing, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 07/12] VMState test: qmp interface for querying the vmstate testing process, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 08/12] VMState test: hmp interface for querying the vmstate testing process, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 09/12] VMState test: update period of vmstate testing process, Sanidhya Kashyap, 2014/07/25
- [Qemu-devel] [PATCH RFC v2 10/12] VMState test: hmp interface for period update, Sanidhya Kashyap, 2014/07/25