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? 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()); \
+ }
struct VfuObjectClass {
ObjectClass parent_class;
--
2.49.0On 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.
> 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();
}
Not really matter. Anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> + }
>
> struct VfuObjectClass {
> ObjectClass parent_class;
--
Best regards,
Vladimir
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;
On 26.09.25 09:51, Markus Armbruster wrote:
> 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();
> }
>
Looks more readable, yes.
> 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.
:)) I'm not an expert in vfio-user at all. But yes, I said it:)
>
> 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?
Looks good.
>
>> Not really matter. Anyway:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Thanks!
>
>>> + }
>>> struct VfuObjectClass {
>>> ObjectClass parent_class;
>
--
Best regards,
Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> On 26.09.25 09:51, Markus Armbruster wrote:
>> 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();
>> }
>>
>
> Looks more readable, yes.
Sold!
>> 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.
>
> :)) I'm not an expert in vfio-user at all. But yes, I said it:)
I'll soften it to "believes 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?
>
> Looks good.
Thanks again!
>>
>>> Not really matter. Anyway:
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Thanks!
>>
>>>> + }
>>>> struct VfuObjectClass {
>>>> ObjectClass parent_class;
>>
On 23/9/25 11: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? 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(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2026 Red Hat, Inc.