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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  tests/test-vmstate.c | 363 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 185 insertions(+), 178 deletions(-)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index a462335..d0839c3 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -272,217 +272,226 @@ static void test_simple_primitive(void)
>  }
>  #undef FIELD_EQUAL
> 
> -typedef struct TestStruct {
> +typedef struct TestVersioned {
>      uint32_t a, b, c, e;
>      uint64_t d, f;
>      bool skip_c_e;
> -} TestStruct;
> +} TestVersioned;
> +
> +TestVersioned obj_versioned = {
> +    .a = 10,
> +    .b = 200,
> +    .c = 30,
> +    .d = 40,
> +    .e = 500,
> +    .f = 600,
> +    .skip_c_e = true,
> +};
> 
> -static const VMStateDescription vmstate_versioned = {
> +static const VMStateDescription vmstate_simple_versioned = {
>      .name = "test/versioned",
>      .version_id = 2,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(a, TestStruct),
> -        VMSTATE_UINT32_V(b, TestStruct, 2), /* Versioned field in the middle, so
> -                                             * we catch bugs more easily.
> -                                             */
> -        VMSTATE_UINT32(c, TestStruct),
> -        VMSTATE_UINT64(d, TestStruct),
> -        VMSTATE_UINT32_V(e, TestStruct, 2),
> -        VMSTATE_UINT64_V(f, TestStruct, 2),
> +        VMSTATE_UINT32(a, TestVersioned),
> +        /* Versioned field in the middle, so we catch bugs more
> +         * easily. */
> +        VMSTATE_UINT32_V(b, TestVersioned, 2),
> +        VMSTATE_UINT32(c, TestVersioned),
> +        VMSTATE_UINT64(d, TestVersioned),
> +        VMSTATE_UINT32_V(e, TestVersioned, 2),
> +        VMSTATE_UINT64_V(f, TestVersioned, 2),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 
> -static void test_load_v1(void)
> +uint8_t wire_simple_v1[] = {
> +    /* a */ 0x00, 0x00, 0x00, 0x0a,
> +    /* c */ 0x00, 0x00, 0x00, 0x1e,
> +    /* d */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x28,
> +    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
> +};
> +
> +uint8_t wire_simple_v2[] = {
> +    /* a */ 0x00, 0x00, 0x00, 0x0a,
> +    /* b */ 0x00, 0x00, 0x00, 0xc8,
> +    /* c */ 0x00, 0x00, 0x00, 0x1e,
> +    /* d */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x28,
> +    /* e */ 0x00, 0x00, 0x01, 0xf4,
> +    /* f */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x58,
> +    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
> +};
> +
> +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?

>  }
> 
> -static void test_load_v2(void)
> +static void test_simple_v2(void)
>  {
> -    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 */
> -        0, 0, 0, 50,             /* e */
> -        0, 0, 0, 0, 0, 0, 0, 60, /* f */
> -        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;
> -    vmstate_load_state(loading, &vmstate_versioned, &obj, 2);
> -    g_assert_cmpint(obj.a, ==, 10);
> -    g_assert_cmpint(obj.b, ==, 20);
> -    g_assert_cmpint(obj.c, ==, 30);
> -    g_assert_cmpint(obj.d, ==, 40);
> -    g_assert_cmpint(obj.e, ==, 50);
> -    g_assert_cmpint(obj.f, ==, 60);
> -    qemu_fclose(loading);
> +    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.

> +    FIELD_EQUAL(a);
> +    FIELD_EQUAL(b);
> +    FIELD_EQUAL(c);
> +    FIELD_EQUAL(d);
> +    FIELD_EQUAL(e);
> +    FIELD_EQUAL(f);
> +}
> +
> +static void test_simple_v1(void)
> +{
> +    TestVersioned obj, obj_clone;
> +
> +    memset(&obj, 0, sizeof(obj));
> +    SUCCESS(load_vmstate(&vmstate_simple_versioned, &obj, &obj_clone,
> +                         obj_versioned_copy, 1, wire_simple_v1,
> +                         sizeof(wire_simple_v1)));
> +
> +    FIELD_EQUAL(a);
> +    FIELD_NOT_EQUAL(b);
> +    FIELD_EQUAL(c);
> +    FIELD_EQUAL(d);
> +    FIELD_NOT_EQUAL(e);
> +    FIELD_NOT_EQUAL(f);
>  }
> 
>  static bool test_skip(void *opaque, int version_id)
>  {
> -    TestStruct *t = (TestStruct *)opaque;
> +    TestVersioned *t = opaque;
>      return !t->skip_c_e;
>  }
> 
> -static const VMStateDescription vmstate_skipping = {
> -    .name = "test/skip",
> +static const VMStateDescription vmstate_simple_skipping = {
> +    .name = "simple/skip",
>      .version_id = 2,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(a, TestStruct),
> -        VMSTATE_UINT32(b, TestStruct),
> -        VMSTATE_UINT32_TEST(c, TestStruct, test_skip),
> -        VMSTATE_UINT64(d, TestStruct),
> -        VMSTATE_UINT32_TEST(e, TestStruct, test_skip),
> -        VMSTATE_UINT64_V(f, TestStruct, 2),
> +        VMSTATE_BOOL(skip_c_e, TestVersioned),
> +        VMSTATE_UINT32(a, TestVersioned),
> +        VMSTATE_UINT32(b, TestVersioned),
> +        VMSTATE_UINT32_TEST(c, TestVersioned, test_skip),
> +        VMSTATE_UINT64(d, TestVersioned),
> +        VMSTATE_UINT32_TEST(e, TestVersioned, test_skip),
> +        VMSTATE_UINT64_V(f, TestVersioned, 2),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 
> +uint8_t wire_simple_no_skip[] = {
> +    /* s */ 0x00,
> +    /* a */ 0x00, 0x00, 0x00, 0x0a,
> +    /* b */ 0x00, 0x00, 0x00, 0xc8,
> +    /* c */ 0x00, 0x00, 0x00, 0x1e,
> +    /* d */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x28,
> +    /* e */ 0x00, 0x00, 0x01, 0xf4,
> +    /* f */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x58,
> +    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
> +};
> 
> -static void test_save_noskip(void)
> -{
> -    QEMUFile *fsave = open_test_file(true);
> -    TestStruct obj = { .a = 1, .b = 2, .c = 3, .d = 4, .e = 5, .f = 6,
> -                       .skip_c_e = false };
> -    vmstate_save_state(fsave, &vmstate_skipping, &obj);
> -    g_assert(!qemu_file_get_error(fsave));
> -    qemu_fclose(fsave);
> -
> -    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 */
> -        0, 0, 0, 5,             /* e */
> -        0, 0, 0, 0, 0, 0, 0, 6, /* f */
> -    };
> -    uint8_t result[sizeof(expected)];
> -    g_assert_cmpint(qemu_get_buffer(loading, result, sizeof(result)), ==,
> -                    sizeof(result));
> -    g_assert(!qemu_file_get_error(loading));
> -    g_assert_cmpint(memcmp(result, expected, sizeof(result)), ==, 0);
> -
> -    /* Must reach EOF */
> -    qemu_get_byte(loading);
> -    g_assert_cmpint(qemu_file_get_error(loading), ==, -EIO);
> -
> -    qemu_fclose(loading);
> -}
> +uint8_t wire_simple_skip[] = {
> +    /* s */ 0x01,
> +    /* a */ 0x00, 0x00, 0x00, 0x0a,
> +    /* b */ 0x00, 0x00, 0x00, 0xc8,
> +    /* d */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x28,
> +    /* f */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x58,
> +    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
> +};
> 
> -static void test_save_skip(void)
> +static void test_simple_no_skip(void)
>  {
> -    QEMUFile *fsave = open_test_file(true);
> -    TestStruct obj = { .a = 1, .b = 2, .c = 3, .d = 4, .e = 5, .f = 6,
> -                       .skip_c_e = true };
> -    vmstate_save_state(fsave, &vmstate_skipping, &obj);
> -    g_assert(!qemu_file_get_error(fsave));
> -    qemu_fclose(fsave);
> -
> -    QEMUFile *loading = open_test_file(false);
> -    uint8_t expected[] = {
> -        0, 0, 0, 1,             /* a */
> -        0, 0, 0, 2,             /* b */
> -        0, 0, 0, 0, 0, 0, 0, 4, /* d */
> -        0, 0, 0, 0, 0, 0, 0, 6, /* f */
> -    };
> -    uint8_t result[sizeof(expected)];
> -    g_assert_cmpint(qemu_get_buffer(loading, result, sizeof(result)), ==,
> -                    sizeof(result));
> -    g_assert(!qemu_file_get_error(loading));
> -    g_assert_cmpint(memcmp(result, expected, sizeof(result)), ==, 0);
> +    TestVersioned obj, obj_clone;
> 
> -
> -    /* Must reach EOF */
> -    qemu_get_byte(loading);
> -    g_assert_cmpint(qemu_file_get_error(loading), ==, -EIO);
> -
> -    qemu_fclose(loading);
> +    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.

> +    SUCCESS(load_vmstate(&vmstate_simple_skipping, &obj, &obj_clone,
> +                         obj_versioned_copy, 1, wire_simple_no_skip,
> +                         sizeof(wire_simple_no_skip) - 8));
> +
> +    FIELD_EQUAL(skip_c_e);
> +    FIELD_EQUAL(a);
> +    FIELD_EQUAL(b);
> +    FIELD_EQUAL(c);
> +    FIELD_EQUAL(d);
> +    FIELD_EQUAL(e);
> +    FIELD_NOT_EQUAL(f);
> +
> +    SUCCESS(load_vmstate(&vmstate_simple_skipping, &obj, &obj_clone,
> +                         obj_versioned_copy, 2, wire_simple_no_skip,
> +                         sizeof(wire_simple_no_skip)));
> +
> +    FIELD_EQUAL(skip_c_e);
> +    FIELD_EQUAL(a);
> +    FIELD_EQUAL(b);
> +    FIELD_EQUAL(c);
> +    FIELD_EQUAL(d);
> +    FIELD_EQUAL(e);
> +    FIELD_EQUAL(f);
>  }
> 
> -static void test_load_noskip(void)
> +static void test_simple_skip(void)
>  {
> -    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 */
> -        0, 0, 0, 50,             /* e */
> -        0, 0, 0, 0, 0, 0, 0, 60, /* f */
> -        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 = { .skip_c_e = false };
> -    vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> -    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);
> -    g_assert_cmpint(obj.e, ==, 50);
> -    g_assert_cmpint(obj.f, ==, 60);
> -    qemu_fclose(loading);
> -}
> +    TestVersioned obj, obj_clone;
> 
> -static void test_load_skip(void)
> -{
> -    QEMUFile *fsave = open_test_file(true);
> -    uint8_t buf[] = {
> -        0, 0, 0, 10,             /* a */
> -        0, 0, 0, 20,             /* b */
> -        0, 0, 0, 0, 0, 0, 0, 40, /* d */
> -        0, 0, 0, 0, 0, 0, 0, 60, /* f */
> -        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 = { .skip_c_e = true, .c = 300, .e = 500 };
> -    vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> -    g_assert(!qemu_file_get_error(loading));
> -    g_assert_cmpint(obj.a, ==, 10);
> -    g_assert_cmpint(obj.b, ==, 20);
> -    g_assert_cmpint(obj.c, ==, 300);
> -    g_assert_cmpint(obj.d, ==, 40);
> -    g_assert_cmpint(obj.e, ==, 500);
> -    g_assert_cmpint(obj.f, ==, 60);
> -    qemu_fclose(loading);
> +    memset(&obj, 0, sizeof(obj));
> +    obj_versioned.skip_c_e = true;
> +    save_vmstate(&vmstate_simple_skipping, &obj_versioned);
> +
> +    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?

> +#undef FIELD_EQUAL
> +#undef FIELD_NOT_EQUAL
> 
>  int main(int argc, char **argv)
>  {
> @@ -490,12 +499,10 @@ int main(int argc, char **argv)
> 
>      g_test_init(&argc, &argv, NULL);
>      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);
> -    g_test_add_func("/vmstate/field_exists/load/skip", test_load_skip);
> -    g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
> -    g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
> +    g_test_add_func("/vmstate/simple/v1", test_simple_v1);
> +    g_test_add_func("/vmstate/simple/v2", test_simple_v2);
> +    g_test_add_func("/vmstate/simple/skip", test_simple_skip);
> +    g_test_add_func("/vmstate/simple/no_skip", test_simple_no_skip);
>      g_test_run();
> 
>      close(temp_fd);
> -- 
> 1.9.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK