Re: [Qemu-devel] [PATCH 5/5] migration: Route more error paths

[Dr. David Alan Gilbert]

* Fam Zheng (address@hidden) wrote:
> On Tue, 09/19 19:00, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > vmstate_save is called in a few places, and vmstate_save_state is
> > called in lots of places.
> > 
> > Route error returns from the easier cases back up;  there are lots
> > of more complex cases where there own error paths need fixing.
> 
> Did you mean s/there/their/ ?

I do; thanks.

> <snip>
> 
> > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> > index e643ac662b..ab3e430c2c 100644
> > --- a/tests/test-vmstate.c
> > +++ b/tests/test-vmstate.c
> > @@ -70,7 +70,7 @@ 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, NULL);
> > +    g_assert(!vmstate_save_state(f, desc, obj, NULL));
> 
> Though this is test code, isn't putting anything with a side effect into an
> assert expression a very bad pattern in general?

Hmm; ok I've changed this but I'm not really convinced; the whole point
of an asser in a test is to actually run it, and I think the g_assert
prints the text that failed, so it gives you a much better error inline.

Dave

> >      qemu_put_byte(f, QEMU_VM_EOF);
> >      g_assert(!qemu_file_get_error(f));
> >      qemu_fclose(f);
> > @@ -381,7 +381,7 @@ 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, NULL);
> > +    g_assert(!vmstate_save_state(fsave, &vmstate_skipping, &obj, NULL));
> >      g_assert(!qemu_file_get_error(fsave));
> >  
> >      uint8_t expected[] = {
> > @@ -402,7 +402,7 @@ static void test_save_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, NULL);
> > +    g_assert(!vmstate_save_state(fsave, &vmstate_skipping, &obj, NULL));
> >      g_assert(!qemu_file_get_error(fsave));
> >  
> >      uint8_t expected[] = {
> > -- 
> > 2.13.5
> > 
> > 
> 
> Fam
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK