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

["Daniel P. Berrangé" <berrange@redhat.com>]

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