diff mbox series

[5/5] migration: Route more error paths

Message ID 20170919180038.26056-6-dgilbert@redhat.com
State New
Headers show
Series migration: let pre_save fail | expand

Commit Message

Dr. David Alan Gilbert Sept. 19, 2017, 6 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/display/virtio-gpu.c    |  4 +---
 hw/virtio/virtio.c         | 13 +++++++------
 include/hw/virtio/virtio.h |  2 +-
 migration/vmstate-types.c  | 11 ++++++++---
 tests/test-vmstate.c       |  6 +++---
 5 files changed, 20 insertions(+), 16 deletions(-)

Comments

Peter Xu Sept. 20, 2017, 3:31 a.m. UTC | #1 
On Tue, Sep 19, 2017 at 07:00:38PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

IIUC we are touching up the put()s hooks.  Then do we need this as
well below?

-----
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 3226e8e..70e1e14 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -355,7 +355,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                 } else if (field->flags & VMS_STRUCT) {
                     vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
                 } else {
-                    field->info->put(f, curr_elem, size, field, vmdesc_loop);
+                    ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
+                    if (ret) {
+                        return ret;
+                    }
                 }
 
                 written_bytes = qemu_ftell_fast(f) - old_offset;
-----

Or these errors won't really stop the migration?

> ---
>  hw/display/virtio-gpu.c    |  4 +---
>  hw/virtio/virtio.c         | 13 +++++++------
>  include/hw/virtio/virtio.h |  2 +-
>  migration/vmstate-types.c  | 11 ++++++++---
>  tests/test-vmstate.c       |  6 +++---
>  5 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 622ee300f9..c9494e8a79 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -996,9 +996,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
>      }
>      qemu_put_be32(f, 0); /* end of list */
>  
> -    vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> -
> -    return 0;
> +    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
>  }
>  
>  static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 464947f76d..860333788b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = {
>      }
>  };
>  
> -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> +int virtio_save(VirtIODevice *vdev, QEMUFile *f)
>  {
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> @@ -1949,20 +1949,21 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      }
>  
>      if (vdc->vmsd) {
> -        vmstate_save_state(f, vdc->vmsd, vdev, NULL);
> +        int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL);
> +        if (ret) {
> +            return ret;
> +        }
>      }
>  
>      /* Subsections */
> -    vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> +    return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
>  }
>  
>  /* A wrapper for use as a VMState .put function */
>  static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
>                                VMStateField *field, QJSON *vmdesc)
>  {
> -    virtio_save(VIRTIO_DEVICE(opaque), f);
> -
> -    return 0;
> +    return virtio_save(VIRTIO_DEVICE(opaque), f);
>  }
>  
>  /* A wrapper for use as a VMState .get function */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 80c45c321e..5abada6966 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -188,7 +188,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>  void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq);
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>  
> -void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> +int virtio_save(VirtIODevice *vdev, QEMUFile *f);
>  
>  extern const VMStateInfo virtio_vmstate_info;
>  
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index c056c98bdb..48184c380d 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -550,13 +550,14 @@ static int put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
>  {
>      const VMStateDescription *vmsd = field->vmsd;
>      void *tmp = g_malloc(size);
> +    int ret;
>  
>      /* Writes the parent field which is at the start of the tmp */
>      *(void **)tmp = pv;
> -    vmstate_save_state(f, vmsd, tmp, vmdesc);
> +    ret = vmstate_save_state(f, vmsd, tmp, vmdesc);
>      g_free(tmp);
>  
> -    return 0;
> +    return ret;
>  }
>  
>  const VMStateInfo vmstate_info_tmp = {
> @@ -657,12 +658,16 @@ static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>      /* offset of the QTAILQ entry in a QTAILQ element*/
>      size_t entry_offset = field->start;
>      void *elm;
> +    int ret;
>  
>      trace_put_qtailq(vmsd->name, vmsd->version_id);
>  
>      QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {
>          qemu_put_byte(f, true);
> -        vmstate_save_state(f, vmsd, elm, vmdesc);
> +        ret = vmstate_save_state(f, vmsd, elm, vmdesc);
> +        if (ret) {
> +            return ret;
> +        }
>      }
>      qemu_put_byte(f, false);
>  
> 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));
>      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 Zheng Sept. 20, 2017, 3:55 a.m. UTC | #2 
On Tue, 09/19 19:00, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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/ ?

<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?

>      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
Eric Blake Sept. 20, 2017, 1:26 p.m. UTC | #3 
On 09/19/2017 10:55 PM, Fam Zheng wrote:
> On Tue, 09/19 19:00, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> 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.
> 

>> +++ 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?

Indeed - although we don't disable asserts (as of commit 262a69f4), this
should still be separated into running vmstate_save_state()
unconditionally, then asserting that the result stored into a temporary
variable matches expectations.
Dr. David Alan Gilbert Sept. 20, 2017, 2:20 p.m. UTC | #4 
* Fam Zheng (famz@redhat.com) wrote:
> On Tue, 09/19 19:00, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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 / dgilbert@redhat.com / Manchester, UK
Cornelia Huck Sept. 20, 2017, 2:30 p.m. UTC | #5 
On Tue, 19 Sep 2017 19:00:38 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/display/virtio-gpu.c    |  4 +---
>  hw/virtio/virtio.c         | 13 +++++++------
>  include/hw/virtio/virtio.h |  2 +-
>  migration/vmstate-types.c  | 11 ++++++++---
>  tests/test-vmstate.c       |  6 +++---
>  5 files changed, 20 insertions(+), 16 deletions(-)
> 

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 464947f76d..860333788b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = {
>      }
>  };
>  
> -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> +int virtio_save(VirtIODevice *vdev, QEMUFile *f)
>  {
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);

Would it make sense to touch up the save_config callback as well? Else
virtio_save() looks a bit lopsided.

[For virtio-ccw, the callback can simply pass on any return code from
vmstate_save_state(). For virtio-pci, we can probably make
pci_device_save() restore the interrupt state in any case.]
Eric Blake Sept. 20, 2017, 3:32 p.m. UTC | #6 
On 09/20/2017 09:20 AM, Dr. David Alan Gilbert wrote:

>>>      /* 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.

glib doesn't make life any easier by having two different flavors of
assertions.

g_assert() can be disabled at compile-time (side effects are lost).  It
is roughly akin to assert(); and the output when an assertion fails
isn't that much more useful than what you get with plain assert().

g_assert_cmpint() cannot be disabled at compile-time, but can have a
runtime switch on whether it abort()s or continues on.  Here, the output
when something fails is MUCH more pleasant to read.

But the fact that there are two separate families of g_assert* behaviors
just makes it easier to live by the mantra that side-effects don't
belong inside assertions.
Dr. David Alan Gilbert Sept. 20, 2017, 3:45 p.m. UTC | #7 
* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Sep 19, 2017 at 07:00:38PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> IIUC we are touching up the put()s hooks.  Then do we need this as
> well below?

You're right - 'put' has returned int since Jianjun's patches but
I seem to have missed that it never wired the check up.

> -----
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 3226e8e..70e1e14 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -355,7 +355,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                  } else if (field->flags & VMS_STRUCT) {
>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>                  } else {
> -                    field->info->put(f, curr_elem, size, field, vmdesc_loop);
> +                    ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
> +                    if (ret) {
> +                        return ret;
> +                    }
>                  }
>  
>                  written_bytes = qemu_ftell_fast(f) - old_offset;
> -----
> 
> Or these errors won't really stop the migration?

Added as a separate patch (with an error report in the if)

Thanks,

Dave

> 
> > ---
> >  hw/display/virtio-gpu.c    |  4 +---
> >  hw/virtio/virtio.c         | 13 +++++++------
> >  include/hw/virtio/virtio.h |  2 +-
> >  migration/vmstate-types.c  | 11 ++++++++---
> >  tests/test-vmstate.c       |  6 +++---
> >  5 files changed, 20 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 622ee300f9..c9494e8a79 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -996,9 +996,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
> >      }
> >      qemu_put_be32(f, 0); /* end of list */
> >  
> > -    vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> > -
> > -    return 0;
> > +    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> >  }
> >  
> >  static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 464947f76d..860333788b 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = {
> >      }
> >  };
> >  
> > -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > +int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >  {
> >      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > @@ -1949,20 +1949,21 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >      }
> >  
> >      if (vdc->vmsd) {
> > -        vmstate_save_state(f, vdc->vmsd, vdev, NULL);
> > +        int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL);
> > +        if (ret) {
> > +            return ret;
> > +        }
> >      }
> >  
> >      /* Subsections */
> > -    vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> > +    return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> >  }
> >  
> >  /* A wrapper for use as a VMState .put function */
> >  static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
> >                                VMStateField *field, QJSON *vmdesc)
> >  {
> > -    virtio_save(VIRTIO_DEVICE(opaque), f);
> > -
> > -    return 0;
> > +    return virtio_save(VIRTIO_DEVICE(opaque), f);
> >  }
> >  
> >  /* A wrapper for use as a VMState .get function */
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 80c45c321e..5abada6966 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -188,7 +188,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> >  void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq);
> >  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
> >  
> > -void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> > +int virtio_save(VirtIODevice *vdev, QEMUFile *f);
> >  
> >  extern const VMStateInfo virtio_vmstate_info;
> >  
> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> > index c056c98bdb..48184c380d 100644
> > --- a/migration/vmstate-types.c
> > +++ b/migration/vmstate-types.c
> > @@ -550,13 +550,14 @@ static int put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
> >  {
> >      const VMStateDescription *vmsd = field->vmsd;
> >      void *tmp = g_malloc(size);
> > +    int ret;
> >  
> >      /* Writes the parent field which is at the start of the tmp */
> >      *(void **)tmp = pv;
> > -    vmstate_save_state(f, vmsd, tmp, vmdesc);
> > +    ret = vmstate_save_state(f, vmsd, tmp, vmdesc);
> >      g_free(tmp);
> >  
> > -    return 0;
> > +    return ret;
> >  }
> >  
> >  const VMStateInfo vmstate_info_tmp = {
> > @@ -657,12 +658,16 @@ static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> >      /* offset of the QTAILQ entry in a QTAILQ element*/
> >      size_t entry_offset = field->start;
> >      void *elm;
> > +    int ret;
> >  
> >      trace_put_qtailq(vmsd->name, vmsd->version_id);
> >  
> >      QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {
> >          qemu_put_byte(f, true);
> > -        vmstate_save_state(f, vmsd, elm, vmdesc);
> > +        ret = vmstate_save_state(f, vmsd, elm, vmdesc);
> > +        if (ret) {
> > +            return ret;
> > +        }
> >      }
> >      qemu_put_byte(f, false);
> >  
> > 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));
> >      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
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Sept. 20, 2017, 3:57 p.m. UTC | #8 
* Cornelia Huck (cohuck@redhat.com) wrote:
> On Tue, 19 Sep 2017 19:00:38 +0100
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/display/virtio-gpu.c    |  4 +---
> >  hw/virtio/virtio.c         | 13 +++++++------
> >  include/hw/virtio/virtio.h |  2 +-
> >  migration/vmstate-types.c  | 11 ++++++++---
> >  tests/test-vmstate.c       |  6 +++---
> >  5 files changed, 20 insertions(+), 16 deletions(-)
> > 
> 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 464947f76d..860333788b 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = {
> >      }
> >  };
> >  
> > -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > +int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >  {
> >      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> 
> Would it make sense to touch up the save_config callback as well? Else
> virtio_save() looks a bit lopsided.
> 
> [For virtio-ccw, the callback can simply pass on any return code from
> vmstate_save_state(). For virtio-pci, we can probably make
> pci_device_save() restore the interrupt state in any case.]

What I'd really love to do with save_config is turn it into a
VMStateDescription* - I think I've figured out how to
do that for ccw and mmio - but the pci version is a much harder
mess of casts and stuff.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Cornelia Huck Sept. 20, 2017, 4 p.m. UTC | #9 
On Wed, 20 Sep 2017 16:57:39 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Cornelia Huck (cohuck@redhat.com) wrote:
> > On Tue, 19 Sep 2017 19:00:38 +0100
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> >   
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  hw/display/virtio-gpu.c    |  4 +---
> > >  hw/virtio/virtio.c         | 13 +++++++------
> > >  include/hw/virtio/virtio.h |  2 +-
> > >  migration/vmstate-types.c  | 11 ++++++++---
> > >  tests/test-vmstate.c       |  6 +++---
> > >  5 files changed, 20 insertions(+), 16 deletions(-)
> > >   
> >   
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 464947f76d..860333788b 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1899,7 +1899,7 @@ static const VMStateDescription vmstate_virtio = {
> > >      }
> > >  };
> > >  
> > > -void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > > +int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > >  {
> > >      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> > >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);  
> > 
> > Would it make sense to touch up the save_config callback as well? Else
> > virtio_save() looks a bit lopsided.
> > 
> > [For virtio-ccw, the callback can simply pass on any return code from
> > vmstate_save_state(). For virtio-pci, we can probably make
> > pci_device_save() restore the interrupt state in any case.]  
> 
> What I'd really love to do with save_config is turn it into a
> VMStateDescription* - I think I've figured out how to
> do that for ccw and mmio - but the pci version is a much harder
> mess of casts and stuff.

Sounds even better, but even more complicated as well...
diff mbox series

Patch

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 622ee300f9..c9494e8a79 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -996,9 +996,7 @@  static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
     }
     qemu_put_be32(f, 0); /* end of list */
 
-    vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
-
-    return 0;
+    return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
 }
 
 static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 464947f76d..860333788b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1899,7 +1899,7 @@  static const VMStateDescription vmstate_virtio = {
     }
 };
 
-void virtio_save(VirtIODevice *vdev, QEMUFile *f)
+int virtio_save(VirtIODevice *vdev, QEMUFile *f)
 {
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
@@ -1949,20 +1949,21 @@  void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     }
 
     if (vdc->vmsd) {
-        vmstate_save_state(f, vdc->vmsd, vdev, NULL);
+        int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL);
+        if (ret) {
+            return ret;
+        }
     }
 
     /* Subsections */
-    vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
+    return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
 }
 
 /* A wrapper for use as a VMState .put function */
 static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
                               VMStateField *field, QJSON *vmdesc)
 {
-    virtio_save(VIRTIO_DEVICE(opaque), f);
-
-    return 0;
+    return virtio_save(VIRTIO_DEVICE(opaque), f);
 }
 
 /* A wrapper for use as a VMState .get function */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 80c45c321e..5abada6966 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -188,7 +188,7 @@  void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq);
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
-void virtio_save(VirtIODevice *vdev, QEMUFile *f);
+int virtio_save(VirtIODevice *vdev, QEMUFile *f);
 
 extern const VMStateInfo virtio_vmstate_info;
 
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index c056c98bdb..48184c380d 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -550,13 +550,14 @@  static int put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
 {
     const VMStateDescription *vmsd = field->vmsd;
     void *tmp = g_malloc(size);
+    int ret;
 
     /* Writes the parent field which is at the start of the tmp */
     *(void **)tmp = pv;
-    vmstate_save_state(f, vmsd, tmp, vmdesc);
+    ret = vmstate_save_state(f, vmsd, tmp, vmdesc);
     g_free(tmp);
 
-    return 0;
+    return ret;
 }
 
 const VMStateInfo vmstate_info_tmp = {
@@ -657,12 +658,16 @@  static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
     /* offset of the QTAILQ entry in a QTAILQ element*/
     size_t entry_offset = field->start;
     void *elm;
+    int ret;
 
     trace_put_qtailq(vmsd->name, vmsd->version_id);
 
     QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {
         qemu_put_byte(f, true);
-        vmstate_save_state(f, vmsd, elm, vmdesc);
+        ret = vmstate_save_state(f, vmsd, elm, vmdesc);
+        if (ret) {
+            return ret;
+        }
     }
     qemu_put_byte(f, false);
 
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));
     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[] = {