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

[Juan Quintela <quintela@redhat.com>]

Tejus GK <tejus.gk@nutanix.com> wrote:
> 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.

Ouch, that again.

I think that I know how to fix that.

Will take a look.

Later, Juan.