[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 010/124] vmstate: Refactor & increase tests for
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH 010/124] vmstate: Refactor & increase tests for primitive types |
Date: |
Tue, 22 Apr 2014 17:26:15 +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:
>> 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 :-(
>> /* 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 / address@hidden / Manchester, UK
[Qemu-devel] [PATCH 008/124] vmstate: Reduce code duplication, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 009/124] vmstate: Refactor opening of files, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 010/124] vmstate: Refactor & increase tests for primitive types, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 011/124] vmstate: Port versioned tests to new format, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 012/124] vmstate: Create test functions for versions until 15, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 013/124] vmstate: Remove VMSTATE_UINTL_EQUAL_V, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 015/124] vmstate: Remove unused VMSTATE_UINTTL_ARRAY_V, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 014/124] vmstate: Change VMSTATE_INTTL_V to VMSTATE_INTTL_TEST, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 016/124] vmstate: Test for VMSTATE_BOOL_TEST, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 017/124] vmstate: Test for VMSTATE_INT8_TEST, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 018/124] vmstate: Test for VMSTATE_INT16_TEST, Juan Quintela, 2014/04/21
[Qemu-devel] [PATCH 019/124] vmstate: Test for VMSTATE_INT32_TEST, Juan Quintela, 2014/04/21