Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new format

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>

>> +static void obj_versioned_copy(void *arg1, void *arg2)
>>  {
>> -    QEMUFile *fsave = open_test_file(true);
>> -    uint8_t buf[] = {
>> -        0, 0, 0, 10,             /* a */
>> -        0, 0, 0, 30,             /* c */
>> -        0, 0, 0, 0, 0, 0, 0, 40, /* d */
>> -        QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>> -    };
>> -    qemu_put_buffer(fsave, buf, sizeof(buf));
>> -    qemu_fclose(fsave);
>> -
>> -    QEMUFile *loading = open_test_file(false);
>> -    TestStruct obj = { .b = 200, .e = 500, .f = 600 };
>> -    vmstate_load_state(loading, &vmstate_versioned, &obj, 1);
>> -    g_assert(!qemu_file_get_error(loading));
>> -    g_assert_cmpint(obj.a, ==, 10);
>> -    g_assert_cmpint(obj.b, ==, 200);
>> -    g_assert_cmpint(obj.c, ==, 30);
>> -    g_assert_cmpint(obj.d, ==, 40);
>> -    g_assert_cmpint(obj.e, ==, 500);
>> -    g_assert_cmpint(obj.f, ==, 600);
>> -    qemu_fclose(loading);
>> +    TestVersioned *target = arg1;
>> +    TestVersioned *source = arg2;
>> +
>> +    target->a = source->a;
>> +    target->b = source->b;
>> +    target->c = source->c;
>> +    target->d = source->d;
>> +    target->e = source->e;
>> +    target->f = source->f;
>> +    target->skip_c_e = source->skip_c_e;
>
> Why's that not simply *target = *source?

To be able to only copy some of the fields, depending on what we want to
test O:-)

>> +    TestVersioned obj, obj_clone;
>> +
>> +    memset(&obj, 0, sizeof(obj));
>> +    save_vmstate(&vmstate_simple_versioned, &obj_versioned);
>> +
>> +    compare_vmstate(wire_simple_v2, sizeof(wire_simple_v2));
>> +
>> +    SUCCESS(load_vmstate(&vmstate_simple_versioned, &obj, &obj_clone,
>> +                         obj_versioned_copy, 2, wire_simple_v2,
>> +                         sizeof(wire_simple_v2)));
>> +
>> +#define FIELD_EQUAL(name)   g_assert_cmpint(obj.name, ==, obj_versioned.name)
>> +#define FIELD_NOT_EQUAL(name) \
>> +    g_assert_cmpint(obj.name, !=, obj_versioned.name)
>
> Given that macro is shared with the next few functions it would be seem
> to declare it outside of the function.
>

It is not always the same (see the whole series otherwise).  I ended
finding easier to do it this way.  I ended defining it the 1st time that
I needed it.  That is coherent with the whole series, but I can change
it (don't really matter).


>> +    memset(&obj, 0, sizeof(obj));
>> +    obj_versioned.skip_c_e = false;
>> +    save_vmstate(&vmstate_simple_skipping, &obj_versioned);
>> +
>> +    compare_vmstate(wire_simple_no_skip, sizeof(wire_simple_no_skip));
>> +
>> +    /* We abuse the fact that f has a 0x00 value in the right position */
>
> A bit nasty.


Any better ideas?


>> +    compare_vmstate(wire_simple_skip, sizeof(wire_simple_skip));
>> +
>> +    /* We abuse the fact that f has a 0x00 value in the right position */
>> +    SUCCESS(load_vmstate(&vmstate_simple_skipping, &obj, &obj_clone,
>> +                         obj_versioned_copy, 1, wire_simple_skip,
>> +                         sizeof(wire_simple_skip) - 8));
>> +
>> +    FIELD_EQUAL(skip_c_e);
>> +    FIELD_EQUAL(a);
>> +    FIELD_EQUAL(b);
>> +    FIELD_NOT_EQUAL(c);
>> +    FIELD_EQUAL(d);
>> +    FIELD_NOT_EQUAL(e);
>> +    FIELD_NOT_EQUAL(f);
>> +
>> +    SUCCESS(load_vmstate(&vmstate_simple_skipping, &obj, &obj_clone,
>> +                         obj_versioned_copy, 2, wire_simple_skip,
>> +                         sizeof(wire_simple_skip)));
>> +
>> +    FIELD_EQUAL(skip_c_e);
>> +    FIELD_EQUAL(a);
>> +    FIELD_EQUAL(b);
>> +    FIELD_NOT_EQUAL(c);
>> +    FIELD_EQUAL(d);
>> +    FIELD_NOT_EQUAL(e);
>> +    FIELD_EQUAL(f);

>>  }
>
> Couldn't those functions just be merged and take a flag?
>I think it is clear this way, but that is the same to me.

FIELD_EQUAL(a)
FIELD_NOT_EQUAL(a)

vs

COMPARE_FIELD(a, true)
COMPARE_FILED(b, false)

For me, I don't need to go to the definition of the macro in the 1st
case, and I do on the second one.

Or do you mean anything different?

Thanks, Juan.