[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new form
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new format |
Date: |
Wed, 25 Jun 2014 15:51:38 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
"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.
- [Qemu-devel] [PATCH 02/27] vmstate: Return error in case of error, (continued)
[Qemu-devel] [PATCH 06/27] vmstate: Remove VMSTATE_UINTL_EQUAL_V, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 05/27] vmstate: Create test functions for versions until 15, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 07/27] vmstate: Change VMSTATE_INTTL_V to VMSTATE_INTTL_TEST, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 08/27] vmstate: Remove unused VMSTATE_UINTTL_ARRAY_V, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 10/27] vmstate: Test for VMSTATE_INT8_TEST, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 11/27] vmstate: Test for VMSTATE_INT16_TEST, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 09/27] vmstate: Test for VMSTATE_BOOL_TEST, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 13/27] vmstate: test for VMSTATE_INT64_TEST, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 12/27] vmstate: Test for VMSTATE_INT32_TEST, Juan Quintela, 2014/06/16
[Qemu-devel] [PATCH 14/27] vmstate: Test for VMSTATE_UINT8_TEST, Juan Quintela, 2014/06/16