1. Patchew
  2. QEMU
  3. [PATCH v7 00/24] migration: propagate vTPM errors using Error objects
  4. View patch

[PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()

Arun Menon posted 24 patches 3 months, 3 weeks ago
Maintainers: Stefan Berger <stefanb@linux.vnet.ibm.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Nicholas Piggin <npiggin@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Hailiang Zhang <zhanghailiang@xfusion.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Steve Sistare <steven.sistare@oracle.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
There is a newer version of this series
[PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Arun Menon 3 months, 3 weeks ago 
This is an incremental step in converting vmstate loading
code to report error via Error objects instead of directly
printing it to console/monitor.
It is ensured that vmstate_subsection_load() must report an error
in errp, in case of failure.

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 migration/vmstate.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque, JSONWriter *vmdesc,
                                    Error **errp);
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
-                                   void *opaque);
+                                   void *opaque, Error **errp);
 
 /* Whether this field should exist for either save or load the VM? */
 static bool
@@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         field++;
     }
     assert(field->flags == VMS_END);
-    ret = vmstate_subsection_load(f, vmsd, opaque);
+    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
     if (ret != 0) {
         qemu_file_set_error(f, ret);
         return ret;
@@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
 }
 
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
-                                   void *opaque)
+                                   void *opaque, Error **errp)
 {
     trace_vmstate_subsection_load(vmsd->name);
 
@@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
         if (sub_vmsd == NULL) {
             trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
+            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
+                       idstr, vmsd->name);
             return -ENOENT;
         }
         qemu_file_skip(f, 1); /* subsection */
@@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
         if (ret) {
             trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
+            error_setg(errp,
+                       "Loading VM subsection '%s' in '%s' failed : %d",
+                       idstr, vmsd->name, ret);
             return ret;
         }
     }

-- 
2.50.0
Re: [PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Marc-André Lureau 3 months, 3 weeks ago 
On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <armenon@redhat.com> wrote:

> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that vmstate_subsection_load() must report an error
> in errp, in case of failure.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/vmstate.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index
> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b
> 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
> VMStateDescription *vmsd,
>                                     void *opaque, JSONWriter *vmdesc,
>                                     Error **errp);
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> *vmsd,
> -                                   void *opaque);
> +                                   void *opaque, Error **errp);
>
>  /* Whether this field should exist for either save or load the VM? */
>  static bool
> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const
> VMStateDescription *vmsd,
>          field++;
>      }
>      assert(field->flags == VMS_END);
> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> +    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
>      if (ret != 0) {
>          qemu_file_set_error(f, ret);
>          return ret;
> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription *
> const *sub,
>  }
>
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> *vmsd,
> -                                   void *opaque)
> +                                   void *opaque, Error **errp)
>  {
>      trace_vmstate_subsection_load(vmsd->name);
>
> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
> VMStateDescription *vmsd,
>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>          if (sub_vmsd == NULL) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> "(lookup)");
> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> +                       idstr, vmsd->name);
>              return -ENOENT;
>          }
>          qemu_file_skip(f, 1); /* subsection */
> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const
> VMStateDescription *vmsd,
>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
>          if (ret) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> "(child)");
> +            error_setg(errp,
> +                       "Loading VM subsection '%s' in '%s' failed : %d",
>

extra space before ":"

other than that
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> +                       idstr, vmsd->name, ret);
>              return ret;
>          }
>      }
>
> --
> 2.50.0
>
>
Re: [PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Marc-André Lureau 3 months, 2 weeks ago 
Hi

On Fri, Jul 25, 2025 at 5:46 PM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

>
>
> On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <armenon@redhat.com> wrote:
>
>> This is an incremental step in converting vmstate loading
>> code to report error via Error objects instead of directly
>> printing it to console/monitor.
>> It is ensured that vmstate_subsection_load() must report an error
>> in errp, in case of failure.
>>
>> Signed-off-by: Arun Menon <armenon@redhat.com>
>> ---
>>  migration/vmstate.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index
>> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b
>> 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
>> VMStateDescription *vmsd,
>>                                     void *opaque, JSONWriter *vmdesc,
>>                                     Error **errp);
>>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
>> *vmsd,
>> -                                   void *opaque);
>> +                                   void *opaque, Error **errp);
>>
>>  /* Whether this field should exist for either save or load the VM? */
>>  static bool
>> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const
>> VMStateDescription *vmsd,
>>          field++;
>>      }
>>      assert(field->flags == VMS_END);
>> -    ret = vmstate_subsection_load(f, vmsd, opaque);
>> +    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
>>      if (ret != 0) {
>>          qemu_file_set_error(f, ret);
>>          return ret;
>> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription *
>> const *sub,
>>  }
>>
>>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
>> *vmsd,
>> -                                   void *opaque)
>> +                                   void *opaque, Error **errp)
>>  {
>>      trace_vmstate_subsection_load(vmsd->name);
>>
>> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
>> VMStateDescription *vmsd,
>>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>>          if (sub_vmsd == NULL) {
>>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
>> "(lookup)");
>> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
>> +                       idstr, vmsd->name);
>>              return -ENOENT;
>>          }
>>          qemu_file_skip(f, 1); /* subsection */
>> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const
>> VMStateDescription *vmsd,
>>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
>>          if (ret) {
>>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
>> "(child)");
>> +            error_setg(errp,
>> +                       "Loading VM subsection '%s' in '%s' failed : %d",
>>
>
> extra space before ":"
>
> other than that
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Actually, almost systematically when you introduce an extra **errp
argument, you should ERRP_GUARD() in the function (see include/qapi/error.h
doc). Was this discussed before? Can you update the following patches too?


>
>> +                       idstr, vmsd->name, ret);
>>              return ret;
>>          }
>>      }
>>
>> --
>> 2.50.0
>>
>>
Re: [PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Arun Menon 3 months, 2 weeks ago 
Hi Marc-André,

Thanks for the review.

On Mon, Jul 28, 2025 at 12:44:53PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jul 25, 2025 at 5:46 PM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
> >
> >
> > On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <armenon@redhat.com> wrote:
> >
> >> This is an incremental step in converting vmstate loading
> >> code to report error via Error objects instead of directly
> >> printing it to console/monitor.
> >> It is ensured that vmstate_subsection_load() must report an error
> >> in errp, in case of failure.
> >>
> >> Signed-off-by: Arun Menon <armenon@redhat.com>
> >> ---
> >>  migration/vmstate.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index
> >> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b
> >> 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>                                     void *opaque, JSONWriter *vmdesc,
> >>                                     Error **errp);
> >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> >> *vmsd,
> >> -                                   void *opaque);
> >> +                                   void *opaque, Error **errp);
> >>
> >>  /* Whether this field should exist for either save or load the VM? */
> >>  static bool
> >> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>          field++;
> >>      }
> >>      assert(field->flags == VMS_END);
> >> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> >> +    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
> >>      if (ret != 0) {
> >>          qemu_file_set_error(f, ret);
> >>          return ret;
> >> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription *
> >> const *sub,
> >>  }
> >>
> >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> >> *vmsd,
> >> -                                   void *opaque)
> >> +                                   void *opaque, Error **errp)
> >>  {
> >>      trace_vmstate_subsection_load(vmsd->name);
> >>
> >> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
> >>          if (sub_vmsd == NULL) {
> >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> >> "(lookup)");
> >> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> >> +                       idstr, vmsd->name);
> >>              return -ENOENT;
> >>          }
> >>          qemu_file_skip(f, 1); /* subsection */
> >> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> >>          if (ret) {
> >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> >> "(child)");
> >> +            error_setg(errp,
> >> +                       "Loading VM subsection '%s' in '%s' failed : %d",
> >>
> >
> > extra space before ":"
> >
> > other than that
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> 
> Actually, almost systematically when you introduce an extra **errp
> argument, you should ERRP_GUARD() in the function (see include/qapi/error.h
> doc). Was this discussed before? Can you update the following patches too?

I see. Thanks. I shall add ERRP_GUARD() whenever errp is dereferenced or used with
error_prepend().

> 
> 
> >
> >> +                       idstr, vmsd->name, ret);
> >>              return ret;
> >>          }
> >>      }
> >>
> >> --
> >> 2.50.0
> >>
> >>

Regards,
Arun
Re: [PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Daniel P. Berrangé 3 months, 2 weeks ago 
On Mon, Jul 28, 2025 at 12:44:53PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jul 25, 2025 at 5:46 PM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
> >
> >
> > On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <armenon@redhat.com> wrote:
> >
> >> This is an incremental step in converting vmstate loading
> >> code to report error via Error objects instead of directly
> >> printing it to console/monitor.
> >> It is ensured that vmstate_subsection_load() must report an error
> >> in errp, in case of failure.
> >>
> >> Signed-off-by: Arun Menon <armenon@redhat.com>
> >> ---
> >>  migration/vmstate.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index
> >> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b
> >> 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>                                     void *opaque, JSONWriter *vmdesc,
> >>                                     Error **errp);
> >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> >> *vmsd,
> >> -                                   void *opaque);
> >> +                                   void *opaque, Error **errp);
> >>
> >>  /* Whether this field should exist for either save or load the VM? */
> >>  static bool
> >> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>          field++;
> >>      }
> >>      assert(field->flags == VMS_END);
> >> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> >> +    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
> >>      if (ret != 0) {
> >>          qemu_file_set_error(f, ret);
> >>          return ret;
> >> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription *
> >> const *sub,
> >>  }
> >>
> >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> >> *vmsd,
> >> -                                   void *opaque)
> >> +                                   void *opaque, Error **errp)
> >>  {
> >>      trace_vmstate_subsection_load(vmsd->name);
> >>
> >> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
> >>          if (sub_vmsd == NULL) {
> >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> >> "(lookup)");
> >> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> >> +                       idstr, vmsd->name);
> >>              return -ENOENT;
> >>          }
> >>          qemu_file_skip(f, 1); /* subsection */
> >> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const
> >> VMStateDescription *vmsd,
> >>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> >>          if (ret) {
> >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> >> "(child)");
> >> +            error_setg(errp,
> >> +                       "Loading VM subsection '%s' in '%s' failed : %d",
> >>
> >
> > extra space before ":"
> >
> > other than that
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> 
> Actually, almost systematically when you introduce an extra **errp
> argument, you should ERRP_GUARD() in the function (see include/qapi/error.h
> doc). Was this discussed before? Can you update the following patches too?

ERRP_GUARD is only needed in functions which derefence errp, which should
very rarely be needed when all functions are non-void return value.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Marc-André Lureau 3 months, 2 weeks ago 
Hi

On Mon, Jul 28, 2025 at 1:06 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jul 28, 2025 at 12:44:53PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Jul 25, 2025 at 5:46 PM Marc-André Lureau <
> > marcandre.lureau@redhat.com> wrote:
> >
> > >
> > >
> > > On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <armenon@redhat.com> wrote:
> > >
> > >> This is an incremental step in converting vmstate loading
> > >> code to report error via Error objects instead of directly
> > >> printing it to console/monitor.
> > >> It is ensured that vmstate_subsection_load() must report an error
> > >> in errp, in case of failure.
> > >>
> > >> Signed-off-by: Arun Menon <armenon@redhat.com>
> > >> ---
> > >>  migration/vmstate.c | 11 ++++++++---
> > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> > >> index
> > >> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b
> > >> 100644
> > >> --- a/migration/vmstate.c
> > >> +++ b/migration/vmstate.c
> > >> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
> > >> VMStateDescription *vmsd,
> > >>                                     void *opaque, JSONWriter *vmdesc,
> > >>                                     Error **errp);
> > >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> > >> *vmsd,
> > >> -                                   void *opaque);
> > >> +                                   void *opaque, Error **errp);
> > >>
> > >>  /* Whether this field should exist for either save or load the VM? */
> > >>  static bool
> > >> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const
> > >> VMStateDescription *vmsd,
> > >>          field++;
> > >>      }
> > >>      assert(field->flags == VMS_END);
> > >> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> > >> +    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
> > >>      if (ret != 0) {
> > >>          qemu_file_set_error(f, ret);
> > >>          return ret;
> > >> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription *
> > >> const *sub,
> > >>  }
> > >>
> > >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> > >> *vmsd,
> > >> -                                   void *opaque)
> > >> +                                   void *opaque, Error **errp)
> > >>  {
> > >>      trace_vmstate_subsection_load(vmsd->name);
> > >>
> > >> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
> > >> VMStateDescription *vmsd,
> > >>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
> > >>          if (sub_vmsd == NULL) {
> > >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> > >> "(lookup)");
> > >> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> > >> +                       idstr, vmsd->name);
> > >>              return -ENOENT;
> > >>          }
> > >>          qemu_file_skip(f, 1); /* subsection */
> > >> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const
> > >> VMStateDescription *vmsd,
> > >>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> > >>          if (ret) {
> > >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> > >> "(child)");
> > >> +            error_setg(errp,
> > >> +                       "Loading VM subsection '%s' in '%s' failed : %d",
> > >>
> > >
> > > extra space before ":"
> > >
> > > other than that
> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> >
> > Actually, almost systematically when you introduce an extra **errp
> > argument, you should ERRP_GUARD() in the function (see include/qapi/error.h
> > doc). Was this discussed before? Can you update the following patches too?
>
> ERRP_GUARD is only needed in functions which derefence errp, which should
> very rarely be needed when all functions are non-void return value.

But also, it avoids this pitfall with @errp argument:

* - It should not be passed to error_prepend(), error_vprepend(), or
 *   error_append_hint(), because that doesn't work with &error_fatal.

either way, I don't care much, but for consistency it sounds
reasonable to ask every new function with **errp to have it.

>
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>


-- 
Marc-André Lureau
Re: [PATCH v7 01/24] migration: push Error **errp into vmstate_subsection_load()
Posted by Daniel P. Berrangé 3 months, 2 weeks ago 
On Mon, Jul 28, 2025 at 01:59:40PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 28, 2025 at 1:06 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Jul 28, 2025 at 12:44:53PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Fri, Jul 25, 2025 at 5:46 PM Marc-André Lureau <
> > > marcandre.lureau@redhat.com> wrote:
> > >
> > > >
> > > >
> > > > On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <armenon@redhat.com> wrote:
> > > >
> > > >> This is an incremental step in converting vmstate loading
> > > >> code to report error via Error objects instead of directly
> > > >> printing it to console/monitor.
> > > >> It is ensured that vmstate_subsection_load() must report an error
> > > >> in errp, in case of failure.
> > > >>
> > > >> Signed-off-by: Arun Menon <armenon@redhat.com>
> > > >> ---
> > > >>  migration/vmstate.c | 11 ++++++++---
> > > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> > > >> index
> > > >> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b
> > > >> 100644
> > > >> --- a/migration/vmstate.c
> > > >> +++ b/migration/vmstate.c
> > > >> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
> > > >> VMStateDescription *vmsd,
> > > >>                                     void *opaque, JSONWriter *vmdesc,
> > > >>                                     Error **errp);
> > > >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> > > >> *vmsd,
> > > >> -                                   void *opaque);
> > > >> +                                   void *opaque, Error **errp);
> > > >>
> > > >>  /* Whether this field should exist for either save or load the VM? */
> > > >>  static bool
> > > >> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const
> > > >> VMStateDescription *vmsd,
> > > >>          field++;
> > > >>      }
> > > >>      assert(field->flags == VMS_END);
> > > >> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> > > >> +    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
> > > >>      if (ret != 0) {
> > > >>          qemu_file_set_error(f, ret);
> > > >>          return ret;
> > > >> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription *
> > > >> const *sub,
> > > >>  }
> > > >>
> > > >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> > > >> *vmsd,
> > > >> -                                   void *opaque)
> > > >> +                                   void *opaque, Error **errp)
> > > >>  {
> > > >>      trace_vmstate_subsection_load(vmsd->name);
> > > >>
> > > >> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
> > > >> VMStateDescription *vmsd,
> > > >>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
> > > >>          if (sub_vmsd == NULL) {
> > > >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> > > >> "(lookup)");
> > > >> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> > > >> +                       idstr, vmsd->name);
> > > >>              return -ENOENT;
> > > >>          }
> > > >>          qemu_file_skip(f, 1); /* subsection */
> > > >> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const
> > > >> VMStateDescription *vmsd,
> > > >>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> > > >>          if (ret) {
> > > >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr,
> > > >> "(child)");
> > > >> +            error_setg(errp,
> > > >> +                       "Loading VM subsection '%s' in '%s' failed : %d",
> > > >>
> > > >
> > > > extra space before ":"
> > > >
> > > > other than that
> > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > >
> > > Actually, almost systematically when you introduce an extra **errp
> > > argument, you should ERRP_GUARD() in the function (see include/qapi/error.h
> > > doc). Was this discussed before? Can you update the following patches too?
> >
> > ERRP_GUARD is only needed in functions which derefence errp, which should
> > very rarely be needed when all functions are non-void return value.
> 
> But also, it avoids this pitfall with @errp argument:
> 
> * - It should not be passed to error_prepend(), error_vprepend(), or
>  *   error_append_hint(), because that doesn't work with &error_fatal.

Oh, i forgot about that gotcha :-(

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.