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>
---
Changes in v2:
- dropped qemu_file_set_error_obj(f, ret, local_err);
include/migration/register.h | 3 ++-
hw/ppc/spapr.c | 2 +-
hw/s390x/s390-stattrib.c | 2 +-
hw/vfio/migration.c | 2 +-
migration/block-dirty-bitmap.c | 2 +-
migration/block.c | 2 +-
migration/ram.c | 3 ++-
migration/savevm.c | 2 +-
8 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/migration/register.h b/include/migration/register.h
index 2cc71ec45f65bf2884c9e7a823d2968752f15c20..96eae9dba2970552c379c732393e3ab6ef578a58 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -60,10 +60,11 @@ typedef struct SaveVMHandlers {
*
* @f: QEMUFile where to send the data
* @opaque: data pointer passed to register_savevm_live()
+ * @errp: pointer to Error*, to store an error if it happens.
*
* Returns zero to indicate success and negative for error
*/
- int (*save_setup)(QEMUFile *f, void *opaque);
+ int (*save_setup)(QEMUFile *f, void *opaque, Error **errp);
/**
* @save_cleanup
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 55263f0815ed7671b32ea20b394ae71c82e616cb..045c024ffa76eacfc496bd486cb6cafbee2df73e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = {
}
};
-static int htab_save_setup(QEMUFile *f, void *opaque)
+static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
SpaprMachineState *spapr = opaque;
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);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp)
return 0;
}
-static int vfio_save_setup(QEMUFile *f, void *opaque)
+static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1213,7 +1213,7 @@ fail:
return ret;
}
-static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
+static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
DBMSaveState *s = &((DBMState *)opaque)->save;
SaveBitmapState *dbms = NULL;
diff --git a/migration/block.c b/migration/block.c
index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque)
blk_mig_unlock();
}
-static int block_save_setup(QEMUFile *f, void *opaque)
+static int block_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
int ret;
diff --git a/migration/ram.c b/migration/ram.c
index 4649a8120492a03d331d660622e1a0a51adb0a96..745482899e18c86b73261b683c1bec04039a76d2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2930,8 +2930,9 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
*
* @f: QEMUFile where to send the data
* @opaque: RAMState pointer
+ * @errp: pointer to Error*, to store an error if it happens.
*/
-static int ram_save_setup(QEMUFile *f, void *opaque)
+static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
{
RAMState **rsp = opaque;
RAMBlock *block;
diff --git a/migration/savevm.c b/migration/savevm.c
index bc168371a31acf85f29f2c284be181250db45df4..b5b3b51bad94dc4c04ae22cd687ba111299339aa 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1341,7 +1341,7 @@ 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) {
qemu_file_set_error(f, ret);
--
2.43.2
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.
Questions?
[...]
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.
On 29.02.24 09: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,
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
On 29/02/2024 08.20, Vladimir Sementsov-Ogievskiy wrote:
> On 29.02.24 09: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,
>
> Personally, I'd prefer not doing so. Creating wrong commits and fixing them
> in same series - better to merge all fixes into bad commit:)
I agree - that might create issues with bisecting later. Please fix it in
this patch here already!
Thanks,
Thomas
Thomas Huth <thuth@redhat.com> writes: > On 29/02/2024 08.20, Vladimir Sementsov-Ogievskiy wrote: >> On 29.02.24 09:32, Markus Armbruster wrote: [...] >>> 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:) > > I agree - that might create issues with bisecting later. Please fix it in this patch here already! Depends on the wrongness, really. We don't want broken intermediate states, no argument. But intermediate states that are merely unclean can be acceptable. For instance, my commit a30ecde6e79 (net: Permit incremental conversion of init functions to Error) added such Error ** parameters to a somewhat tangled nest of functions, along with FIXME comments where errors weren't set. The next few commits fixed most, but not all of them. Later commits fixed some more. The one in tap-win32.c is still there today. This was acceptable, because it improved things from "bad error reporting" to "okay error reporting in most cases, unclean and bad error reporting in a few cases marked FIXME", with "a few" over time going down to the one I can't test, and nobody else seems to care about.
On 29.02.24 16:21, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> On 29/02/2024 08.20, Vladimir Sementsov-Ogievskiy wrote:
>>> On 29.02.24 09:32, Markus Armbruster wrote:
>
> [...]
>
>>>> 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:)
>>
>> I agree - that might create issues with bisecting later. Please fix it in this patch here already!
>
> Depends on the wrongness, really.
In my understanding, unset errp on failure path is wrong enough.
For example, simple pattern
Error *local_err = NULL;
int ret = foo(&local_err);
if (ret < 0) {
error_report_err(local_err);
return;
}
will just crash.
When I write code, I expect that "errp is set" === "ret < 0", for all functions returning negative integer on failure.
Also, we have enough code that relying on errp for failure checking:
$ git grep 'if (local_err)' | wc -l
373
Of course, most of this should be checking failure of functions that return void, but who knows.
>
> We don't want broken intermediate states, no argument.
>
> But intermediate states that are merely unclean can be acceptable.
>
> For instance, my commit a30ecde6e79 (net: Permit incremental conversion
> of init functions to Error) added such Error ** parameters to a somewhat
> tangled nest of functions, along with FIXME comments where errors
> weren't set. The next few commits fixed most, but not all of them.
> Later commits fixed some more. The one in tap-win32.c is still there
> today.
>
> This was acceptable, because it improved things from "bad error
> reporting" to "okay error reporting in most cases, unclean and bad error
> reporting in a few cases marked FIXME", with "a few" over time going
> down to the one I can't test, and nobody else seems to care about.
>
--
Best regards,
Vladimir
On 01.03.24 17:44, Vladimir Sementsov-Ogievskiy wrote:
> On 29.02.24 16:21, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> On 29/02/2024 08.20, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 29.02.24 09:32, Markus Armbruster wrote:
>>
>> [...]
>>
>>>>> 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:)
>>>
>>> I agree - that might create issues with bisecting later. Please fix it in this patch here already!
>>
>> Depends on the wrongness, really.
>
> In my understanding, unset errp on failure path is wrong enough.
>
> For example, simple pattern
>
> Error *local_err = NULL;
>
> int ret = foo(&local_err);
> if (ret < 0) {
> error_report_err(local_err);
> return;
> }
>
> will just crash.
>
> When I write code, I expect that "errp is set" === "ret < 0", for all functions returning negative integer on failure.
>
> Also, we have enough code that relying on errp for failure checking:
> $ git grep 'if (local_err)' | wc -l
> 373
>
> Of course, most of this should be checking failure of functions that return void, but who knows.
>
>>
>> We don't want broken intermediate states, no argument.
>>
>> But intermediate states that are merely unclean can be acceptable.
>>
>> For instance, my commit a30ecde6e79 (net: Permit incremental conversion
>> of init functions to Error) added such Error ** parameters to a somewhat
>> tangled nest of functions, along with FIXME comments where errors
>> weren't set. The next few commits fixed most, but not all of them.
>> Later commits fixed some more. The one in tap-win32.c is still there
>> today.
>>
>> This was acceptable, because it improved things from "bad error
>> reporting" to "okay error reporting in most cases, unclean and bad error
>> reporting in a few cases marked FIXME", with "a few" over time going
>> down to the one I can't test, and nobody else seems to care about.
>>
You may be sure, that functions you modify are never used in conditions I've described above. But you can't guarantee that this will not change in future. Of course, if someone will create new call of the function, he should look (at least once) at that function and see "FIXME", but better not rely on this. Also, if someone will add a call to another function that calls function with bad error reporting, most probably he will not see the "FIXME"...
Formally, you should add such FIXME to all functions with errp, that may do (nested) calls to a function with FIXME
--
Best regards,
Vladimir
© 2016 - 2024 Red Hat, Inc.