Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler

["Cédric Le Goater" <clg@redhat.com>]

On 3/7/24 10:53, Vladimir Sementsov-Ogievskiy wrote:
> On 06.03.24 16:34, Cédric Le Goater wrote:
>> The purpose is to record a potential error in the migration stream if
>> qemu_savevm_state_setup() fails. Most of the current .save_setup()
>> handlers can be modified to use the Error argument instead of managing
>> their own and calling locally error_report().
>>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> Cc: Halil Pasic <pasic@linux.ibm.com>
>> Cc: Thomas Huth <thuth@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Cc: John Snow <jsnow@redhat.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Still, if you resend, please add error_prepend in the case below:
> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>>           }
>>           save_section_header(f, se, QEMU_VM_SECTION_START);
>> -        ret = se->ops->save_setup(f, se->opaque);
>> +        ret = se->ops->save_setup(f, se->opaque, errp);
>>           save_section_footer(f, se);
>>           if (ret < 0) {
>> -            error_setg(errp, "failed to setup SaveStateEntry with id(name): "
>> -                       "%d(%s): %d", se->section_id, se->idstr, ret);
> 
> You drop a good bit of information, let's use error_prepend to save it.

I kind of agree but the call stack is quite deep and the callees also use
error_prepend. The error becomes quite long. Here's an example of what we
get today :

   (qemu) migrate -d tcp:10.8.3.15:1234
   (qemu)
   (qemu) qemu-system-x86_64: vfio: Could not start dirty page tracking - 0000:b1:00.2: Failed to start DMA logging: Invalid argument

If the subsystems implementing a .save_setup() handler use a component
identifier, the failure should be fairly easy to identify.

What's the best practice for such cases ?

Can we use multiline errors maybe ? Less practical for grep though.

May be a verbose error mode would help getting more information ?

Anyhow, I can add a new trace event for "failed to setup SaveStateEntry ... "
or use error_prepend() as you suggested.

Let's see what the others have to say.


> 
>>               qemu_file_set_error(f, ret);
>>               break;
> 
> Not about this patch:
> 
> Better do explicit "return ret" instead of this "break" (and one more break above in that loop):
>
> 1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more "cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto fail;".
> 2. "break" make me think, that there may be more logic after it, which will probably fail, and I should be careful, as errp is already set (and second attempt to set it will crash). Again, "goto fail;" is better, as I don't expect more failures when see it.

Sure. If I respin, I can drop the break and simply return. Although,
I would be glad to have most of this series merged in QEMU 9.0. So,
unless there is something major, I will keep that for followups.


Thanks for the review,

C.