Re: [PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Markus Armbruster <armbru@redhat.com>]

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;