On 26.09.25 09:51, Markus Armbruster wrote:
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();
}
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 <[email protected]>
Thanks!
+ }
struct VfuObjectClass {
ObjectClass parent_class;
--
Best regards,
Vladimir
