Vladimir Sementsov-Ogievskiy <[email protected]> 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 <[email protected]> >> Signed-off-by: Markus Armbruster <[email protected]> >> --- >> 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 <[email protected]> Thanks! >> + } >> struct VfuObjectClass { >> ObjectClass parent_class;
