Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Wed, 25 Jan 2017 13:22:55 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Cornelia Huck (cornelia.huck@de.ibm.com) wrote:
> > On Wed, 25 Jan 2017 12:00:53 +0000
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > 
> > > * Fam Zheng (famz@redhat.com) wrote:
> > > > On Tue, 01/24 18:47, Dr. David Alan Gilbert (git) wrote:
> > > > > diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> > > > > index c313166..da8e4df 100644
> > > > > --- a/hw/intc/s390_flic_kvm.c
> > > > > +++ b/hw/intc/s390_flic_kvm.c
> > > > > @@ -286,7 +286,8 @@ static void kvm_s390_release_adapter_routes(S390FLICState *fs,
> > > > >   * increase until buffer is sufficient or maxium size is
> > > > >   * reached
> > > > >   */
> > > > > -static void kvm_flic_save(QEMUFile *f, void *opaque, size_t size)
> > > > > +static int kvm_flic_save(QEMUFile *f, void *opaque, size_t size,
> > > > > +                         VMStateField *field, QJSON *vmdesc)
> > > > >  {
> > > > >      KVMS390FLICState *flic = opaque;
> > > > >      int len = FLIC_SAVE_INITIAL_SIZE;
> > > > > @@ -319,6 +320,8 @@ static void kvm_flic_save(QEMUFile *f, void *opaque, size_t size)
> > > > >                          count * sizeof(struct kvm_s390_irq));
> > > > >      }
> > > > >      g_free(buf);
> > > > > +
> > > > > +    return 0;
> > > > >  }
> > > > 
> > > > This hunk left one 'return' behind in the function, which should have been
> > > > changed to 'return 0' as well, and now the compiler is not happy:
> > > > 
> > > > /var/tmp/patchew-tester-tmp-itftfkl9/src/hw/intc/s390_flic_kvm.c: In function ‘kvm_flic_save’:
> > > > /var/tmp/patchew-tester-tmp-itftfkl9/src/hw/intc/s390_flic_kvm.c:306:9: error: ‘return’ with no value, in function returning non-void [-Werror]
> > > >          return;
> > > >          ^~~~~~
> > > > /var/tmp/patchew-tester-tmp-itftfkl9/src/hw/intc/s390_flic_kvm.c:289:12: note: declared here
> > > >  static int kvm_flic_save(QEMUFile *f, void *opaque, size_t size,
> > > >             ^~~~~~~~~~~~~
> > > > cc1: all warnings being treated as errors
> > > 
> > > OK, so it looks like that's a failure path, adding a return -ENOMEM would seem to make
> > > sense there.
> > 
> > Just saw this. I don't think we want -ENOMEM, as that would change the
> > actual state being saved, no?
> 
> But isn't that the intention of this function?
> 
>     buf = g_try_malloc0(len);
>     if (!buf) {
>         /* Storing FLIC_FAILED into the count field here will cause the
>          * target system to fail when attempting to load irqs from the
>          * migration state */
>         error_report("flic: couldn't allocate memory");
>         qemu_put_be64(f, FLIC_FAILED);
>         return;
>     }
> 
> What should happen on the destination - should the migration fail?
> If we want the migration to fail then we should now return an error
> status rather than 0, and then we see a failed migration on the source
> as well.

Yes. There's also another error case below where we should return an
error instead of putting FLIC_FAILED, then.

The problem is that this is rather hard to test: So I'd prefer to fix
the compile for now and introduce error return codes in a separate
patch.