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

[Cornelia Huck]

On Fri, 27 Jan 2017 18:20:36 +0000
"Dr. David Alan Gilbert" <address@hidden> wrote:

> * Cornelia Huck (address@hidden) wrote:
> > On Wed, 25 Jan 2017 14:44:20 +0000
> > "Dr. David Alan Gilbert" <address@hidden> wrote:
> > 
> > > * Cornelia Huck (address@hidden) wrote:
> > > > On Wed, 25 Jan 2017 13:22:55 +0000
> > > > "Dr. David Alan Gilbert" <address@hidden> wrote:
> > > > 
> > > > > * Cornelia Huck (address@hidden) wrote:
> > > > > > On Wed, 25 Jan 2017 12:00:53 +0000
> > > > > > "Dr. David Alan Gilbert" <address@hidden> 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.