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

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> "Dr. David Alan Gilbert" <address@hidden> wrote:
>> > * Juan Quintela (address@hidden) wrote:
>> >> Signed-off-by: Juan Quintela <address@hidden>
>> 
>> >> +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:-)
>
> But those last 7 lines do copy every field don't they?

They do.

>> >> +    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).
>
> OK.
>
>> >> +    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?
>
> Not really; but it's very reliant on the current format - although perhaps
> demonstrates just how delicate that is.

Yeap, anyways, that is how the test was written, nothing to do with the
refactoring.  Improvements on top welcome O:-)



>> >> +    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?
>
> I meant test_simple_no_skip vs test_simple_skip; they seem to duplicate a lot.

They were there on the original test, i.e. doesn't mater for the
refactoring.  Basically we are doing is testing the "test" function O:-)

Later, Juan.