[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/97] vmstate: Refactor & increase tests for pr
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 06/97] vmstate: Refactor & increase tests for primitive types |
Date: |
Mon, 7 Apr 2014 14:57:30 -0300 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Apr 07, 2014 at 05:20:24AM +0200, Juan Quintela 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 | 195
> +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 150 insertions(+), 45 deletions(-)
>
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 8b242c4..39a769e 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -54,80 +54,186 @@ 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;
> +/* 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 = {
> - .name = "test",
> +static const VMStateDescription vmstate_simple_primitive = {
> + .name = "simple/primitive",
> .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_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()
> }
> };
>
> -static void test_simple_save(void)
> +/* 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 */
> +};
Nice job. I was too lazy to test every single type. :-)
> +
> +#define SUCCESS(val) \
> + g_assert_cmpint((val), ==, 0)
Isn't this the same as g_assert(!val)?
> +
> +#define FAILURE(val) \
> + g_assert_cmpint((val), !=, 0)
Isn't this the same as g_assert(val)?
I know, it is my fault as I have written the g_assert_cmpint(..., 0)
code in the first place. :)
> +
> +static void test_simple_primitive(void)
> {
> QEMUFile *fsave = open_test_file(true);
> - TestStruct obj = { .a = 1, .b = 2, .c = 3, .d = 4 };
> - vmstate_save_state(fsave, &vmstate_simple, &obj);
> +
> + /* Save file with vmstate */
> + vmstate_save_state(fsave, &vmstate_simple_primitive, &obj_simple);
> 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 */
> - };
> - uint8_t result[sizeof(expected)];
> + /* we don't need QEMU_VM_EOF */
> + uint8_t result[sizeof(wire_simple_primitive)-1];
> +
> + /* read back as binary */
> +
> 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);
> +
> + /* Compare that what is on the file is the same that what we
> + expected to be there */
> + SUCCESS(memcmp(result, wire_simple_primitive, sizeof(result)));
>
> /* Must reach EOF */
> qemu_get_byte(loading);
> g_assert_cmpint(qemu_file_get_error(loading), ==, -EIO);
>
> qemu_fclose(loading);
> -}
>
> -static void test_simple_load(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 */
> - QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely
> */
> - };
> - qemu_put_buffer(fsave, buf, sizeof(buf));
> + /* We save the file again. We want the EOF this time */
> +
> + fsave = open_test_file(true);
> + qemu_put_buffer(fsave, wire_simple_primitive,
> + sizeof(wire_simple_primitive));
> qemu_fclose(fsave);
>
> - QEMUFile *loading = open_test_file(false);
> - TestStruct obj;
> - vmstate_load_state(loading, &vmstate_simple, &obj, 1);
> + loading = open_test_file(false);
> + TestSimple obj;
> + SUCCESS(vmstate_load_state(loading, &vmstate_simple_primitive, &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);
> +
> +#define FIELD_ASSERT(name) g_assert_cmpint(obj.name, ==, obj_simple.name)
> + FIELD_ASSERT(b_1);
> + FIELD_ASSERT(b_2);
> + FIELD_ASSERT(u8_1);
> + FIELD_ASSERT(u16_1);
> + FIELD_ASSERT(u32_1);
> + FIELD_ASSERT(u64_1);
> + FIELD_ASSERT(i8_1);
> + FIELD_ASSERT(i8_2);
> + FIELD_ASSERT(i16_1);
> + FIELD_ASSERT(i16_2);
> + FIELD_ASSERT(i32_1);
> + FIELD_ASSERT(i32_2);
> + FIELD_ASSERT(i64_1);
> + FIELD_ASSERT(i64_2);
> +#undef FIELD_ASSERT
I wonder if we could do something to allow us to safely simplify this to
use memcmp(). Looks good, anyway.
> +
> + /* We save the file again. We want the EOF this time */
> +
> + fsave = open_test_file(true);
> + /* this time we don't write the whole file */
> + qemu_put_buffer(fsave, wire_simple_primitive, 10);
> + qemu_fclose(fsave);
What about not just testing if it fails with 10 bytes, but checking all
possible lengths from 0 to N-1 bytes? With this check we are only
testing if the u64 loader handles EOF as expected. If subtle bugs are
going to be introduced in EOF checking, I find them more likely in
corner cases such as length==0, length==1 and lenght==N-1.
> +
> + loading = open_test_file(false);
> + FAILURE(vmstate_load_state(loading, &vmstate_simple_primitive, &obj, 1));
> +
> + /* Now it has to have an error */
> + g_assert(qemu_file_get_error(loading));
> qemu_fclose(loading);
> }
>
> +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 +308,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 +443,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
>
>
--
Eduardo
- [Qemu-devel] [PATCH for 2.1 00/97] VMState simplification (massive), Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 01/97] savevm: Ignore minimum_version_id_old if there is no load_state_old, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 03/97] vmstate: return error in case of error, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 05/97] vmstate: refactor opening of files, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 07/97] vmstate: create test functions for versions until 15, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 04/97] vmstate: reduce code duplication, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 06/97] vmstate: Refactor & increase tests for primitive types, Juan Quintela, 2014/04/06
- Re: [Qemu-devel] [PATCH 06/97] vmstate: Refactor & increase tests for primitive types,
Eduardo Habkost <=
- [Qemu-devel] [PATCH 08/97] vmstate: Remove VMSTATE_UINTL_EQUAL_V, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 09/97] vmstate: Change VMSTATE_INTTL_V to VMSTATE_INTTL_TEST, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 10/97] vmstate: Remove unused VMSTATE_UINTTL_ARRAY_V, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 11/97] vmstate: Test for VMSTATE_BOOL_TEST, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 12/97] vmstate: Test for VMSTATE_INT8_TEST, Juan Quintela, 2014/04/06
- [Qemu-devel] [PATCH 02/97] savevm: Remove all the unneded version_minimum_id_old (Massive), Juan Quintela, 2014/04/06