Re: [Qemu-devel] [PATCH 010/124] vmstate: Refactor & increase tests for primitive types

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> wrote:
> > * Juan Quintela (address@hidden) wrote:
> >> This commit refactor the simple tests to test all integer types. We
> >> move to hex because it is easier to read values of different types.
> >> 
> >> Signed-off-by: Juan Quintela <address@hidden>
> >> ---
> >>  tests/test-vmstate.c | 277 
> >> +++++++++++++++++++++++++++++++++++++++------------
> >>  1 file changed, 216 insertions(+), 61 deletions(-)
> >> 
> >> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> >> index 8b242c4..caf90ec 100644
> >> --- a/tests/test-vmstate.c
> >> +++ b/tests/test-vmstate.c
> >> @@ -54,80 +54,236 @@ static QEMUFile *open_test_file(bool write)
> >>      return qemu_fdopen(fd, write ? "wb" : "rb");
> >>  }
> >> 
> >> -typedef struct TestSruct {
> >> -    uint32_t a, b, c, e;
> >> -    uint64_t d, f;
> >> -    bool skip_c_e;
> >> -} TestStruct;
> >> +#define SUCCESS(val) \
> >> +    g_assert_cmpint((val), ==, 0)
> >> 
> >> +#define FAILURE(val) \
> >> +    g_assert_cmpint((val), !=, 0)
> >> 
> >> -static const VMStateDescription vmstate_simple = {
> >> -    .name = "test",
> >> -    .version_id = 1,
> >> -    .minimum_version_id = 1,
> >> -    .fields = (VMStateField[]) {
> >> -        VMSTATE_UINT32(a, TestStruct),
> >> -        VMSTATE_UINT32(b, TestStruct),
> >> -        VMSTATE_UINT32(c, TestStruct),
> >> -        VMSTATE_UINT64(d, TestStruct),
> >> -        VMSTATE_END_OF_LIST()
> >> -    }
> >> -};
> >> +static void save_vmstate(const VMStateDescription *desc, void *obj)
> >> +{
> >> +    QEMUFile *f = open_test_file(true);
> >> +
> >> +    /* Save file with vmstate */
> >> +    vmstate_save_state(f, desc, obj);
> >> +    qemu_put_byte(f, QEMU_VM_EOF);
> >> +    g_assert(!qemu_file_get_error(f));
> >> +    qemu_fclose(f);
> >> +}
> >> 
> >> -static void test_simple_save(void)
> >> +static void compare_vmstate(uint8_t *wire, size_t size)
> >>  {
> >> -    QEMUFile *fsave = open_test_file(true);
> >> -    TestStruct obj = { .a = 1, .b = 2, .c = 3, .d = 4 };
> >> -    vmstate_save_state(fsave, &vmstate_simple, &obj);
> >> -    g_assert(!qemu_file_get_error(fsave));
> >> -    qemu_fclose(fsave);
> >> +    QEMUFile *f = open_test_file(false);
> >> +    uint8_t result[size];
> >> 
> >> -    QEMUFile *loading = open_test_file(false);
> >> -    uint8_t expected[] = {
> >> -        0, 0, 0, 1, /* a */
> >> -        0, 0, 0, 2, /* b */
> >> -        0, 0, 0, 3, /* c */
> >> -        0, 0, 0, 0, 0, 0, 0, 4, /* d */
> >> -    };
> >> -    uint8_t result[sizeof(expected)];
> >> -    g_assert_cmpint(qemu_get_buffer(loading, result, sizeof(result)), ==,
> >> +    /* read back as binary */
> >> +
> >> +    g_assert_cmpint(qemu_get_buffer(f, result, sizeof(result)), ==,
> >>                      sizeof(result));
> >> -    g_assert(!qemu_file_get_error(loading));
> >> -    g_assert_cmpint(memcmp(result, expected, sizeof(result)), ==, 0);
> >> +    g_assert(!qemu_file_get_error(f));
> >> +
> >> +    /* Compare that what is on the file is the same that what we
> >> +       expected to be there */
> >> +    SUCCESS(memcmp(result, wire, sizeof(result)));
> >
> > To be honest I prefer an explicit memcmp(...) == 0  to the SUCCESS macro 
> > that
> > I have to check it's sense; but that's just my preference.
> 
> 80 lines limit was related to this :-(

The ' == 0' is only 5 characters, but yes the macro removes a lot of the rest.

> >>      /* Must reach EOF */
> >> -    qemu_get_byte(loading);
> >> -    g_assert_cmpint(qemu_file_get_error(loading), ==, -EIO);
> >> +    qemu_get_byte(f);
> >> +    g_assert_cmpint(qemu_file_get_error(f), ==, -EIO);
> >> 
> >> -    qemu_fclose(loading);
> >> +    qemu_fclose(f);
> >>  }
> >> 
> >> -static void test_simple_load(void)
> >> +static int load_vmstate_one(const VMStateDescription *desc, void *obj,
> >> +                            int version, uint8_t *wire, size_t size)
> >>  {
> >> -    QEMUFile *fsave = open_test_file(true);
> >> -    uint8_t buf[] = {
> >> -        0, 0, 0, 10,             /* a */
> >> -        0, 0, 0, 20,             /* b */
> >> -        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 *f;
> >> +    int ret;
> >> +
> >> +    f = open_test_file(true);
> >> +    qemu_put_buffer(f, wire, size);
> >> +    qemu_fclose(f);
> >> +
> >> +    f = open_test_file(false);
> >> +    ret = vmstate_load_state(f, desc, obj, version);
> >> +    if (ret) {
> >> +        g_assert(qemu_file_get_error(f));
> >> +    } else{
> >> +        g_assert(!qemu_file_get_error(f));
> >> +    }
> >> +    qemu_fclose(f);
> >> +    return ret;
> >> +}
> >> 
> >> -    QEMUFile *loading = open_test_file(false);
> >> -    TestStruct obj;
> >> -    vmstate_load_state(loading, &vmstate_simple, &obj, 1);
> >> -    g_assert(!qemu_file_get_error(loading));
> >> -    g_assert_cmpint(obj.a, ==, 10);
> >> -    g_assert_cmpint(obj.b, ==, 20);
> >> -    g_assert_cmpint(obj.c, ==, 30);
> >> -    g_assert_cmpint(obj.d, ==, 40);
> >> -    qemu_fclose(loading);
> >> +
> >> +static int load_vmstate(const VMStateDescription *desc,
> >> +                        void *obj, void *obj_clone,
> >> +                        void (*obj_copy)(void *, void*),
> >> +                        int version, uint8_t *wire, size_t size)
> >> +{
> >> +    /* We test with zero size */
> >> +    obj_copy(obj_clone, obj);
> >> +    FAILURE(load_vmstate_one(desc, obj, version, wire, 0));
> >> +
> >> +    /* Stream ends with QEMU_EOF, so we need at least 3 bytes to be
> >> +     * able to test in the middle */
> >> +
> >> +    if (size > 3) {
> >> +
> >> +        /* We test with size - 2. We can't test size - 1 due to EOF 
> >> tricks */
> >> +        obj_copy(obj, obj_clone);
> >> +        FAILURE(load_vmstate_one(desc, obj, version, wire, size - 2));
> >> +
> >> +        /* Test with size/2, first half of real state */
> >> +        obj_copy(obj, obj_clone);
> >> +        FAILURE(load_vmstate_one(desc, obj, version, wire, size/2));
> >> +
> >> +        /* Test with size/2, second half of real state */
> >> +        obj_copy(obj, obj_clone);
> >> +        FAILURE(load_vmstate_one(desc, obj, version, wire + (size/2), 
> >> size/2));
> >
> > I guess how these fail will depend partly on luck as to how size/2
> > falls; eg if it's
> > in the middle of a big integer or at the end of one.
> 
> We are asking for "size" long section, and we check that we read (or
> not) the whole lot.  When we are sending buffers/arrays with sizes, we
> assure that VMSTATE_INT32_EQUAL() or similar, so if we set a small
> buffer, we would end with a short read.
> 
> >> +
> >> +    }
> >> +    obj_copy(obj, obj_clone);
> >> +    return load_vmstate_one(desc, obj, version, wire, size);
> >> +}
> >> +
> >> +/* Test struct that we are going to use for our tests */
> >> +
> >> +typedef struct TestSimple {
> >> +    bool     b_1,   b_2;
> >> +    uint8_t  u8_1,  u8_2;
> >> +    uint16_t u16_1, u16_2;
> >> +    uint32_t u32_1, u32_2;
> >> +    uint64_t u64_1, u64_2;
> >> +    int8_t   i8_1,  i8_2;
> >> +    int16_t  i16_1, i16_2;
> >> +    int32_t  i32_1, i32_2;
> >> +    int64_t  i64_1, i64_2;
> >> +} TestSimple;
> >> +
> >> +/* Object instantiation, we are going to use it in more than one test */
> >> +
> >> +TestSimple obj_simple = {
> >> +    .b_1 = true,
> >> +    .b_2 = false,
> >> +    .u8_1 = 129,
> >> +    .u8_1 = 130,
> >
> > typo u8_2 ?   But then why did it pass.....
> 
> good catch.
> 
> >> +    .u16_1 = 512,
> >> +    .u16_2 = 45000,
> >> +    .u32_1 = 70000,
> >> +    .u32_2 = 100000,
> >> +    .u64_1 = 12121212,
> >> +    .u64_2 = 23232323,
> >> +    .i8_1 = 65,
> >> +    .i8_2 = -65,
> >> +    .i16_1 = 512,
> >> +    .i16_2 = -512,
> >> +    .i32_1 = 70000,
> >> +    .i32_2 = -70000,
> >> +    .i64_1 = 12121212,
> >> +    .i64_2 = -12121212,
> >
> > It would be a bit easier if you used hex here to match it up
> > with the expected results below.
> 
> >> +};
> >> +
> >> +/* Description of the values.  If you add a primitive type
> >> +   you are expected to add a test here */
> >> +
> >> +static const VMStateDescription vmstate_simple_primitive = {
> >> +    .name = "simple/primitive",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_BOOL(b_1, TestSimple),
> >> +        VMSTATE_BOOL(b_2, TestSimple),
> >> +        VMSTATE_UINT8(u8_1, TestSimple),
> >
> > Ah - missed u8_2 out - which is why the other typo didn't break it.
> 
> I don't need it.  Notice on the vmstate_simple_test() function.  I am
> "reusing" the same struct for more than one Subsection.
> 
> Basically what I do is to sent
> 
> test_simple_primitive()
> 
> - send one example (u8_1)
> 
> test_simple_test()
> - send(u8_1, test_true);
> - send(u8_2, test_false);
> 
> only u8_1 should appear on the wire.
> 
> Oops, test_simple_test() is added on patch 0016 :p
> 
> I should move then back there.
> (yes, I already did lots of rebasing here until I had a result that I liked.)

I kind of half understand that; patch 16 has what looks like a version of
vmstate_simple_test for bools b_1/b_2 - how does that translate into
your u8_1/u8_2 ?
Either way, it's not obvious, so needs a big comment!


> >> +        VMSTATE_UINT16(u16_1, TestSimple),
> >
> > Also missing u16_2
> 
> 
> 
> >> +        VMSTATE_UINT32(u32_1, TestSimple),
> >
> > Also missing u32_2
> >
> >> +        VMSTATE_UINT64(u64_1, TestSimple),
> >
> > Also missing u64_2
> >
> >> +        VMSTATE_INT8(i8_1, TestSimple),
> >> +        VMSTATE_INT8(i8_2, TestSimple),
> >> +        VMSTATE_INT16(i16_1, TestSimple),
> >> +        VMSTATE_INT16(i16_2, TestSimple),
> >> +        VMSTATE_INT32(i32_1, TestSimple),
> >> +        VMSTATE_INT32(i32_2, TestSimple),
> >> +        VMSTATE_INT64(i64_1, TestSimple),
> >> +        VMSTATE_INT64(i64_2, TestSimple),
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >> +/* It describes what goes through the wire.  Our tests are basically:
> >> +
> >> +   * save test
> >> +     - save a struct a vmstate to a file
> >> +     - read that file back (binary read, no vmstate)
> >> +     - compare it with what we expect to be on the wire
> >> +   * load test
> >> +     - save to the file what we expect to be on the wire
> >> +     - read struct back with vmstate in a different
> >> +     - compare back with the original struct
> >> +*/
> >> +
> >> +uint8_t wire_simple_primitive[] = {
> >> +    /* b_1 */   0x01,
> >> +    /* b_2 */   0x00,
> >> +    /* u8_1 */  0x82,
> >
> > and u8_2 missing here, but also that 0x82 is the u8_2 value - so this
> > should fail because it will mismatch the 129 value that you have declared
> > above?!
> 
> no, we are not sending it, so it dont' appear on the wire.

But your description above makes me think that u8_1 is sent, and u8_1 is 
129/0x81
(or at least will be after you fix the typo!)

> 
> >> +    /* u16_1 */ 0x02, 0x00,
> >
> > missing u16_2
> >
> >> +    /* u32_1 */ 0x00, 0x01, 0x11, 0x70,
> >> +    /* u64_1 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0xb8, 0xf4, 0x7c,
> >
> > and u32_2, u64_2
> >
> >> +    /* i8_1 */  0x41,
> >> +    /* i8_2 */  0xbf,
> >> +    /* i16_1 */ 0x02, 0x00,
> >> +    /* i16_2 */ 0xfe, 0x0,
> >> +    /* i32_1 */ 0x00, 0x01, 0x11, 0x70,
> >> +    /* i32_2 */ 0xff, 0xfe, 0xee, 0x90,
> >> +    /* i64_1 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0xb8, 0xf4, 0x7c,
> >> +    /* i64_2 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0x47, 0x0b, 0x84,
> >> +    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely 
> >> */
> >> +};
> >> +
> >> +static void obj_simple_copy(void *target, void *source)
> >> +{
> >> +    memcpy(target, source, sizeof(TestSimple));
> >> +}
> >> +
> >> +static void test_simple_primitive(void)
> >> +{
> >> +    TestSimple obj, obj_clone;
> >> +
> >> +    memset(&obj, 0, sizeof(obj));
> >> +    save_vmstate(&vmstate_simple_primitive, &obj_simple);
> >> +
> >> +    compare_vmstate(wire_simple_primitive, sizeof(wire_simple_primitive));
> >> +
> >> +    SUCCESS(load_vmstate(&vmstate_simple_primitive, &obj, &obj_clone,
> >> +                         obj_simple_copy, 1, wire_simple_primitive,
> >> +                         sizeof(wire_simple_primitive)));
> >> +
> >> +#define FIELD_EQUAL(name)   g_assert_cmpint(obj.name, ==, obj_simple.name)
> >> +
> >> +    FIELD_EQUAL(b_1);
> >> +    FIELD_EQUAL(b_2);
> >> +    FIELD_EQUAL(u8_1);
> >
> > Missing u8_2
> >
> >> +    FIELD_EQUAL(u16_1);
> >
> > Missing u16_2
> >
> >> +    FIELD_EQUAL(u32_1);
> >
> > Missing u32_2
> >
> >> +    FIELD_EQUAL(u64_1);
> >
> > Missing u64_2
> >
> >> +    FIELD_EQUAL(i8_1);
> >> +    FIELD_EQUAL(i8_2);
> >> +    FIELD_EQUAL(i16_1);
> >> +    FIELD_EQUAL(i16_2);
> >> +    FIELD_EQUAL(i32_1);
> >> +    FIELD_EQUAL(i32_2);
> >> +    FIELD_EQUAL(i64_1);
> >> +    FIELD_EQUAL(i64_2);
> >>  }
> >> +#undef FIELD_EQUAL
> >> +
> >> +typedef struct TestStruct {
> >> +    uint32_t a, b, c, e;
> >> +    uint64_t d, f;
> >
> > Imaginative order.
> 
> This is creativity, so it is all Eduardo fault O:-)
> 
> >> +    bool skip_c_e;
> >> +} TestStruct;
> >> 
> >>  static const VMStateDescription vmstate_versioned = {
> >> -    .name = "test",
> >> +    .name = "test/versioned",
> >>      .version_id = 2,
> >>      .minimum_version_id = 1,
> >>      .fields = (VMStateField[]) {
> >> @@ -202,7 +358,7 @@ static bool test_skip(void *opaque, int version_id)
> >>  }
> >> 
> >>  static const VMStateDescription vmstate_skipping = {
> >> -    .name = "test",
> >> +    .name = "test/skip",
> >>      .version_id = 2,
> >>      .minimum_version_id = 1,
> >>      .fields = (VMStateField[]) {
> >> @@ -337,8 +493,7 @@ int main(int argc, char **argv)
> >>      temp_fd = mkstemp(temp_file);
> >> 
> >>      g_test_init(&argc, &argv, NULL);
> >> -    g_test_add_func("/vmstate/simple/save", test_simple_save);
> >> -    g_test_add_func("/vmstate/simple/load", test_simple_load);
> >> +    g_test_add_func("/vmstate/simple/primitive", test_simple_primitive);
> >>      g_test_add_func("/vmstate/versioned/load/v1", test_load_v1);
> >>      g_test_add_func("/vmstate/versioned/load/v2", test_load_v2);
> >>      g_test_add_func("/vmstate/field_exists/load/noskip", 
> >> test_load_noskip);
> >> -- 
> >> 1.9.0
> >> 
> >> 
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK