Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> On 23.09.25 12:09, Markus Armbruster wrote:
>> VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
>> ...) when auto-shutdown is enabled, else with error_report().
>>
>> Issues:
>>
>> 1. The error is serious enough to warrant aborting the process when
>> auto-shutdown is enabled, yet harmless enough to permit carrying on
>> when it's disabled. This makes no sense to me.
>>
>> 2. Like assert(), &error_abort is strictly for programming errors. Is
>> this one?
>
> Brief look at the code make me think that, no it isn't.
So the use of &error_abort is wrong.
>> Or should we exit(1) instead?
>>
>> 3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
>> assert()."
>>
>> This patch addresses just 3.
>>
>> Cc: Jagannathan Raman <jag.raman@oracle.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> hw/remote/vfio-user-obj.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> index ea6165ebdc..eb96982a3a 100644
>> --- a/hw/remote/vfio-user-obj.c
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -75,12 +75,9 @@ OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>> */
>> #define VFU_OBJECT_ERROR(o, fmt, ...) \
>> { \
>> - if (vfu_object_auto_shutdown()) { \
>> - error_setg(&error_abort, (fmt), ## __VA_ARGS__); \
>> - } else { \
>> - error_report((fmt), ## __VA_ARGS__); \
>> - } \
>> - } \
>> + error_report((fmt), ## __VA_ARGS__); \
>> + assert(!vfu_object_auto_shutdown()); \
>
> Probably, it's only my feeling, but for me, assert() is really strictly bound
> to programming errors, more than abort(). Using abort() for errors which are
> not programming, but we can't handle them looks less confusing, i.e.
>
> if (vfu_object_auto_shutdown()) {
> abort();
> }
assert(COND) is if (COND) abort() plus a message meant to help
developers. Both are for programming errors. If it isn't something
that needs debugging, why dump core?
But this particular error condition is *not* a programming error. So
assert(!vfu_object_auto_shutdown());
and
if (vfu_object_auto_shutdown()) {
abort();
}
are both equally wrong. However, the latter makes it easier to add a
FIXME comment:
if (vfu_object_auto_shutdown()) {
/*
* FIXME This looks inappropriate. The error is serious
* enough programming error to warrant aborting the process
* when auto-shutdown is enabled, yet harmless enough to
* permit carrying on when it's disabled. Makes no sense.
*/
abort();
}
The commit message would then need a tweak. Perhaps
Issues:
1. The error is serious enough to warrant killing the process when
auto-shutdown is enabled, yet harmless enough to permit carrying on
when it's disabled. This makes no sense to me.
2. Like assert(), &error_abort is strictly for programming errors. Is
this one? Vladimir Sementsov-Ogievskiy tells me it's not.
3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
assert()."
This patch addresses just 3. It adds a FIXME comment for the other
two.
Thoughts?
> Not really matter. Anyway:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Thanks!
>> + }
>> struct VfuObjectClass {
>> ObjectClass parent_class;