Re: [PATCH v2 06/21] migration: Add Error** argument to .save_setup() handler

From: "Cédric Le Goater" <clg@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, "Peter Xu" <peterx@redhat.com>,
	"Fabiano Rosas" <farosas@suse.de>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	"Avihai Horon" <avihaih@nvidia.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Nicholas Piggin" <npiggin@gmail.com>,
	"Harsh Prateek Bora" <harshpb@linux.ibm.com>,
	"Halil Pasic" <pasic@linux.ibm.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru>,
	"John Snow" <jsnow@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>
Subject: Re: [PATCH v2 06/21] migration: Add Error** argument to .save_setup() handler
Date: Thu, 29 Feb 2024 14:34:27 +0100	[thread overview]
Message-ID: <356b2acd-cdbd-4517-a4e0-624f9b9484cc@redhat.com> (raw)
In-Reply-To: <87zfvj3hnd.fsf@pond.sub.org>

On 2/29/24 07:32, Markus Armbruster wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> 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(). The following patches
>> will introduce such changes for VFIO first.
>>
>> 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>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
> 
> [...]
> 
>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>> index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644
>> --- a/hw/s390x/s390-stattrib.c
>> +++ b/hw/s390x/s390-stattrib.c
>> @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id)
>>       return ret;
>>   }
>>   
>> -static int cmma_save_setup(QEMUFile *f, void *opaque)
>> +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       S390StAttribState *sas = S390_STATTRIB(opaque);
>>       S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
>         int res;
>         /*
>          * Signal that we want to start a migration, thus needing PGSTE dirty
>          * tracking.
>          */
>         res = sac->set_migrationmode(sas, 1);
>         if (res) {
>             return res;
> 
> I believe this is a failure return.
> 
> Anti-pattern: fail without setting an error.  There might be more
> elsewhere in the series.
> 
> qapi/error.h's big comment:
> 
>   * - On success, the function should not touch *errp.  On failure, it
>   *   should set a new error, e.g. with error_setg(errp, ...), or
>   *   propagate an existing one, e.g. with error_propagate(errp, ...).
>   *
>   * - Whenever practical, also return a value that indicates success /
>   *   failure.  This can make the error checking more concise, and can
>   *   avoid useless error object creation and destruction.  Note that
>   *   we still have many functions returning void.  We recommend
>   *   • bool-valued functions return true on success / false on failure,
>   *   • pointer-valued functions return non-null / null pointer, and
>   *   • integer-valued functions return non-negative / negative.
> 
>         }
>         qemu_put_be64(f, STATTR_FLAG_EOS);
>         return 0;
>     }
> 
> When adding Error **errp to a function, you must also add code to set an
> error on failure to every failure path.  Adding it in a later patch in
> the same series can be okay, but I'd add a TODO comment to the function
> then, and mention it in the commit message.

Indeed. I will check the other changes.

> Questions?

Perfectly clear.

Thanks,

C.