On Fri, 27 Jan 2017 18:20:36 +0000
"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> * Cornelia Huck (cornelia.h...@de.ibm.com) wrote:
> > On Wed, 25 Jan 2017 14:44:20 +0000
> > "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> >
> > > * Cornelia Huck (cornelia.h...@de.ibm.com) wrote:
> > > > On Wed, 25 Jan 2017 13:22:55 +0000
> > > > "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> > > >
> > > > > * Cornelia Huck (cornelia.h...@de.ibm.com) wrote:
> > > > > > On Wed, 25 Jan 2017 12:00:53 +0000
> > > > > > "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> >
> > > > > > > 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.
> > >
> > > OK, that's fair.
> >
> > I've coded something up and tried to test it with error injection to
> > trigger the failed case, but I can't really see an improvement :(
> >
> > Before: source logs error, target fails to load the flic with 'invalid
> > argument'
> >
> > After: source logs error, target fails to load the flic with 'could not
> > allocate memory'
> >
> > The migration code does not seem to do anything with the return code of
> > put methods for now, so that's not too surprising. Is anything in the
> > works?
>
> OK, I hadn't remembered that wasn't wired up.
>
> > For now, I'd prefer to keep the old behaviour as 'invalid argument'
> > seems like a more obvious error on the target.
>
> Yes, agreed.
>
> If you feel like, then I'd just change the return -ENOMEM but still
> send the FLIC_FAILED. That way the destination still fails
> as before, and the destination will fail nicely when we wire up the
> error checks.
Yes, having both FLIC_FAILED and a nonzero return code looks like the
best approach for now.
