Cédric Le Goater <c...@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 <npig...@gmail.com>
> Cc: Harsh Prateek Bora <hars...@linux.ibm.com>
> Cc: Halil Pasic <pa...@linux.ibm.com>
> Cc: Thomas Huth <th...@redhat.com>
> Cc: Eric Blake <ebl...@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
> Cc: John Snow <js...@redhat.com>
> Cc: Stefan Hajnoczi <stefa...@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> Reviewed-by: Peter Xu <pet...@redhat.com>
> Signed-off-by: Cédric Le Goater <c...@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.
Questions?
[...]
