Re: [Qemu-devel] [PATCH 04/27] vmstate: Port versioned tests to new format

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> >> +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?
> 
> To be able to only copy some of the fields, depending on what we want to
> test O:-)

But those last 7 lines do copy every field don't they?

> >> +    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.
> >
> 
> It is not always the same (see the whole series otherwise).  I ended
> finding easier to do it this way.  I ended defining it the 1st time that
> I needed it.  That is coherent with the whole series, but I can change
> it (don't really matter).

OK.

> >> +    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.
> 
> 
> Any better ideas?

Not really; but it's very reliant on the current format - although perhaps
demonstrates just how delicate that is.

> >> +    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?
> >I think it is clear this way, but that is the same to me.
> 
> FIELD_EQUAL(a)
> FIELD_NOT_EQUAL(a)
> 
> vs
> 
> COMPARE_FIELD(a, true)
> COMPARE_FILED(b, false)
> 
> For me, I don't need to go to the definition of the macro in the 1st
> case, and I do on the second one.
> 
> Or do you mean anything different?

I meant test_simple_no_skip vs test_simple_skip; they seem to duplicate a lot.

Dave
> 
> Thanks, Juan.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK