* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> > * Juan Quintela (quint...@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 <quint...@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 / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
