diff mbox

[010/124] vmstate: Refactor & increase tests for primitive types

Message ID 1398091304-10677-11-git-send-email-quintela@redhat.com
State New
Headers show

Commit Message

Juan Quintela April 21, 2014, 2:39 p.m. UTC
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 <quintela@redhat.com>
---
 tests/test-vmstate.c | 277 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 216 insertions(+), 61 deletions(-)

Comments

Dr. David Alan Gilbert April 22, 2014, 2:08 p.m. UTC | #1
* Juan Quintela (quintela@redhat.com) 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 <quintela@redhat.com>
> ---
>  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.

>      /* 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.

> +
> +    }
> +    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.....

> +    .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.

> +        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?!

> +    /* 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.

> +    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 / dgilbert@redhat.com / Manchester, UK
Juan Quintela April 22, 2014, 3:26 p.m. UTC | #2
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) 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 <quintela@redhat.com>
>> ---
>>  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 :-(

>>      /* 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.)

>> +        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.

>> +    /* 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 / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert April 22, 2014, 7:05 p.m. UTC | #3
* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) 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 <quintela@redhat.com>
> >> ---
> >>  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 / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

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)));

     /* 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));
+
+    }
+    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,
+    .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,
+};
+
+/* 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),
+        VMSTATE_UINT16(u16_1, TestSimple),
+        VMSTATE_UINT32(u32_1, TestSimple),
+        VMSTATE_UINT64(u64_1, TestSimple),
+        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,
+    /* u16_1 */ 0x02, 0x00,
+    /* u32_1 */ 0x00, 0x01, 0x11, 0x70,
+    /* u64_1 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0xb8, 0xf4, 0x7c,
+    /* 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);
+    FIELD_EQUAL(u16_1);
+    FIELD_EQUAL(u32_1);
+    FIELD_EQUAL(u64_1);
+    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;
+    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);