From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49382) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wws0V-0006oT-PU for qemu-devel@nongnu.org; Tue, 17 Jun 2014 07:57:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wws0M-0002JQ-1j for qemu-devel@nongnu.org; Tue, 17 Jun 2014 07:56:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33096) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wws0L-0002I5-NN for qemu-devel@nongnu.org; Tue, 17 Jun 2014 07:56:45 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5HBujDU004246 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 17 Jun 2014 07:56:45 -0400 Date: Tue, 17 Jun 2014 12:56:40 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140617115639.GH2503@work-vm> References: <1402912703-28195-1-git-send-email-quintela@redhat.com> <1402912703-28195-5-git-send-email-quintela@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1402912703-28195-5-git-send-email-quintela@redhat.com> Subject: Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org * Juan Quintela (quintela@redhat.com) wrote: > Signed-off-by: Juan Quintela > --- > 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