[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo |
Date: |
Fri, 27 Jan 2017 18:20:36 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* 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.
Dave
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index e86a84e49a..3c62ef8258 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -293,27 +293,21 @@ static int kvm_flic_save(QEMUFile *f, void *opaque,
> size_t size,
> int len = FLIC_SAVE_INITIAL_SIZE;
> void *buf;
> int count;
> + int r = 0;
>
> flic_disable_wait_pfault((struct KVMS390FLICState *) opaque);
>
> 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 0;
> + return -ENOMEM;
> }
>
> count = __get_all_irqs(flic, &buf, len);
> if (count < 0) {
> error_report("flic: couldn't retrieve irqs from kernel, rc %d",
> count);
> - /* Storing FLIC_FAILED into the count field here will cause the
> - * target system to fail when attempting to load irqs from the
> - * migration state */
> - qemu_put_be64(f, FLIC_FAILED);
> + r = count;
> } else {
> qemu_put_be64(f, count);
> qemu_put_buffer(f, (uint8_t *) buf,
> @@ -321,7 +315,7 @@ static int kvm_flic_save(QEMUFile *f, void *opaque,
> size_t size,
> }
> g_free(buf);
>
> - return 0;
> + return r;
> }
>
> /**
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, (continued)
- [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Dr. David Alan Gilbert (git), 2017/01/24
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Fam Zheng, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Dr. David Alan Gilbert, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Cornelia Huck, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Fam Zheng, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Cornelia Huck, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Dr. David Alan Gilbert, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Cornelia Huck, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Dr. David Alan Gilbert, 2017/01/25
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo, Cornelia Huck, 2017/01/26
- Re: [Qemu-devel] [PULL 02/15] migration: extend VMStateInfo,
Dr. David Alan Gilbert <=
[Qemu-devel] [PULL 03/15] migration: migrate QTAILQ, Dr. David Alan Gilbert (git), 2017/01/24
[Qemu-devel] [PULL 08/15] migration: Allow "device add" options to only add migratable devices, Dr. David Alan Gilbert (git), 2017/01/24
[Qemu-devel] [PULL 11/15] migration: re-active images while migration been canceled after inactive them, Dr. David Alan Gilbert (git), 2017/01/24
[Qemu-devel] [PULL 14/15] migration: transform remaining DPRINTF into trace_, Dr. David Alan Gilbert (git), 2017/01/24
[Qemu-devel] [PULL 15/15] migration/tracing: Add tracing on save, Dr. David Alan Gilbert (git), 2017/01/24
Re: [Qemu-devel] [PULL 00/15] migration queue, Peter Maydell, 2017/01/25