Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Tejus GK]

On 04/10/23 1:53 pm, Juan Quintela wrote:
> Tejus GK <tejus.gk@nutanix.com> wrote:
> > On 03/10/23 6:14 pm, Juan Quintela wrote:
> > > Tejus GK <tejus.gk@nutanix.com> wrote:
> > > > A few code paths exist in the source code,where a migration is
> > > > marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
> > > > outside of migration.c
> > > >
> > > > In such cases, an error_report() call is made, however the current
> > > > MigrationState is never updated with the error description, and hence
> > > > clients like libvirt never know the actual reason for the failure.
> > > >
> > > > This patch covers such cases outside of migration.c and updates the
> > > > error description at the appropriate places.
> > > >
> > > > Acked-by: Peter Xu <peterx@redhat.com>
> > > > Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > > Queued.
> > Thanks, will be sending out a patch with the "Reviewed by" trailer added.
>
> Send the other one.  This one is already queued.
>
> I think that the error_report() thing, you need to review more things
> than the one in this patch.

Not sure what you mean here? The only other patch I have on the list 
apart from this one is 
https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg00280.html , 
which you marked as reviewed as well.
> > > >                return ret;
> > > >            }
> > > >        }
> > > > @@ -389,8 +390,8 @@ int vmstate_save_state_v(QEMUFile *f, const 
> > > > VMStateDescription *vmsd,
> > > >                                         vmdesc_loop);
> > > >                    }
> > > >                    if (ret) {
> > > > -                    error_report("Save of field %s/%s failed",
> > > > -                                 vmsd->name, field->name);
> > > > +                    error_setg(errp, "Save of field %s/%s failed",
> > > > +                                vmsd->name, field->name);
> > > Same here.
> > You're right, I'm only setting it here and reporting it eventually in
> > savevm.c. The trivial solution for this would have been directly doing
> > a migrate_set_error() here, but that ended up breaking the build for
> > the unit test test-vmstate.c
>
> What was the error?  Because the problem could be on the test.
This is what I keep getting.

FAILED: tests/unit/test-vmstate
cc -m64 -mcx16  -o tests/unit/test-vmstate 
tests/unit/test-vmstate.p/test-vmstate.c.o -Wl,--as-needed 
-Wl,--no-undefined -pie -Wl,--whole-archive -Wl,--start-group 
libevent-loop-base.a libqom.fa libio.fa libcrypto.fa libauthz.fa 
-Wl,--no-whole-archive -fstack-protector-strong -Wl,-z,relro -Wl,-z,now 
-Wl,--warn-common libqemuutil.a 
subprojects/libvhost-user/libvhost-user-glib.a 
subprojects/libvhost-user/libvhost-user.a libmigration.fa libqom.fa 
libio.fa libcrypto.fa libauthz.fa /usr/lib64/libz.so 
/usr/lib64/libgio-2.0.so /usr/lib64/libgobject-2.0.so 
/usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -pthread -lm 
/usr/lib64/libpixman-1.so -Wl,--end-group
libmigration.fa.p/migration_vmstate.c.o: In function `vmstate_save_state_v':
/rpmbuild/SOURCES/qemu/build/../migration/vmstate.c:333: undefined 
reference to `migrate_get_current'
/rpmbuild/SOURCES/qemu/build/../migration/vmstate.c:344: undefined 
reference to `migrate_set_error'
collect2: error: ld returned 1 exit status

I tried to figure out how the dependencies work out for unit test, but 
found no luck with that.
> > > >                        if (vmsd->post_save) {
> > > >                            vmsd->post_save(opaque);
> > > >                        }
> > > So, I am wondering if it could be better to just report the error in
> > > a
> > > single place for migration, and set it whenever we need it?
> > Yes, that would be very convenient, for all the errors to be reported
> > in lets say migration.c. Though that'd also require all the subsystems
> > under migration.c to properly propagate the errors.
>
> Yeap, it requires auditing all the entry points, but will make easier to
> know when we just need to set the error, the system will do the rest.
>
> Thanks, Juan.

regards,
tejus