On 29.02.24 09:32, Markus Armbruster wrote:
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,
Personally, I'd prefer not doing so. Creating wrong commits and fixing them in
same series - better to merge all fixes into bad commit:)
but I'd add a TODO comment to the function
then, and mention it in the commit message.
Questions?
[...]
--
Best regards,
Vladimir
