Re: [PATCH v6 03/24] migration: push Error **errp into qemu_loadvm_state_header()

[Arun Menon <armenon@redhat.com>]

Hi,
Thanks for the review.

On Mon, Jul 21, 2025 at 04:34:12PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 21, 2025 at 3:30 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 qemu_loadvm_state_header() must report an error
> > in errp, in case of failure.
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > ab947620f724874f325fb9fb59bef50b7c16fb51..162fb05933fae5993eeef107811f97cb08726ac3
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2814,35 +2814,42 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t
> > type)
> >      return 0;
> >  }
> >
> > -static int qemu_loadvm_state_header(QEMUFile *f)
> > +static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
> >  {
> >      unsigned int v;
> >      int ret;
> >
> >      v = qemu_get_be32(f);
> >      if (v != QEMU_VM_FILE_MAGIC) {
> > -        error_report("Not a migration stream");
> > +        error_setg(errp, "Not a migration stream, "
> > +                   "magic: %x != %x", v, QEMU_VM_FILE_MAGIC);
> >          return -EINVAL;
> >      }
> >
> >      v = qemu_get_be32(f);
> >      if (v == QEMU_VM_FILE_VERSION_COMPAT) {
> > -        error_report("SaveVM v2 format is obsolete and don't work
> > anymore");
> > +        error_setg(errp, "SaveVM v2 format is obsolete and no"
> > +                   "longer supported, file version %x != %x",
> >
> 
> As Akihiko said in patch 2 review, better "not to have line breaks in
> string literals" .. here, there is a missing space now.

Agreed. I am going to amend all the commits once and check for these kind of
breaks. However, I do not want to have the line too long also (more than 100 chars).

I will try to find a way in which I can use minimum breaks, while keeping in mind
that a grep search should be able to find the line in the codebase, if that is ok.
> 
> +                   v, QEMU_VM_FILE_VERSION_COMPAT);
> > +
> >          return -ENOTSUP;
> >      }
> >      if (v != QEMU_VM_FILE_VERSION) {
> > -        error_report("Unsupported migration stream version");
> > +        error_setg(errp, "Unsupported migration stream "
> > +                   "version, file version %x != %x", v,
> > QEMU_VM_FILE_VERSION);
> >          return -ENOTSUP;
> >      }
> >
> >      if (migrate_get_current()->send_configuration) {
> > -        if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
> > -            error_report("Configuration section missing");
> > +        v = qemu_get_byte(f);
> > +        if (v != QEMU_VM_CONFIGURATION) {
> > +            error_setg(errp, "Configuration section missing,"
> >
> 
> here too
yes
> 
> 
> > +                       "%x != %x", v, QEMU_VM_CONFIGURATION);
> >              return -EINVAL;
> >          }
> > -        ret = vmstate_load_state(f, &vmstate_configuration,
> > &savevm_state, 0,
> > -                                 NULL);
> >
> > +        ret = vmstate_load_state(f, &vmstate_configuration,
> > &savevm_state, 0,
> > +                                 errp);
> >          if (ret) {
> >              return ret;
> >          }
> > @@ -3119,7 +3126,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >
> >      qemu_loadvm_thread_pool_create(mis);
> >
> > -    ret = qemu_loadvm_state_header(f);
> > +    ret = qemu_loadvm_state_header(f, NULL);
> >      if (ret) {
> >          return ret;
> >      }
> >
> > --
> > 2.50.0
> >
> >
Regards,
Arun