On 12/15/22 07:49, Markus Armbruster wrote:
linux-user/ does not use coroutines, so I'd like to avoid that it
includes qemu/coroutine.h.
They include it even before the patch, via lockable.h.
They do but there's a difference between "including lockable.h and
implictly getting coroutine.h due to dependencies" and "including
coroutine.h when you really wanted QEMU_LOCK_GUARD()".
My patch actually enables*not* including coroutine.h: with it applied,
including lockable.h no longer gets you coroutine.h as well.
If you include lockable.h and make use of certain macros, the compile
fails, and you fix it by including coroutine.h instead like pretty much
everything else. Is this really too objectionable to be committed?
s/certain macros/all macros/. All you can do is
qemu_lockable_lock/unlock, which is the less common usage of
qemu/lockable.h:
- qemu_lockable_lock/unlock: used in include/qemu/seqlock.h,
tests/unit/test-coroutine.c, util/qemu-coroutine-lock.c
- QEMU_LOCK_GUARD and WITH_QEMU_LOCK_GUARD are used in 49 files.
1) qemu/coroutine.h keeps including qemu/lockable.h
As in my patch.
That's where the similarity ends. :)
2) qemu/lockable.h is modified as follows to omit the reference to CoMutex:
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 86db7cb04c9c..db59656538a4 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -71,9 +71,11 @@ qemu_null_lockable(void *x)
void *: qemu_null_lockable(x), \
QemuMutex *: qemu_make_lockable(x, QML_OBJ_(x, mutex)), \
QemuRecMutex *: qemu_make_lockable(x, QML_OBJ_(x,
rec_mutex)), \
- CoMutex *: qemu_make_lockable(x, QML_OBJ_(x, co_mutex)), \
+ QEMU_MAKE_CO_MUTEX_LOCKABLE(x) \
QemuSpin *: qemu_make_lockable(x, QML_OBJ_(x, spin)))
+#define QEMU_MAKE_CO_MUTEX_LOCKABLE(x)
+
/**
* QEMU_MAKE_LOCKABLE_NONNULL - Make a polymorphic QemuLockable
*
3) the following hack is added in qemu/coroutine.h, right after including
qemu/lockable.h:
#undef QEMU_MAKE_CO_MUTEX_LOCKABLE(x)
#define QEMU_MAKE_CO_MUTEX_LOCKABLE(x) \
CoMutex *: qemu_make_lockable(x, QML_OBJ_(x, co_mutex)),
Let me see... if I include just lockable.h and make use of certain
(generic) macro(s), the macro(s) won't have a case for QemuMutex *.
Using them with a QemuMutex * argument won't compile.
s/QemuMutex/CoMutex/. That is, you get the CoMutex case only if you
include qemu/coroutine.h. Which you probably did anyway, because
CoMutexes are almost always embedded (not used as pointers). In fact I
suspect the above trick also makes it possible to remove CoMutex from
qemu/typedefs.h.
Furthermore, using qemu_lockable_lock/unlock with CoMutexes still works,
even if you don't include qemu/coroutine.h, just like in your patch.
Neither is particularly pretty, so I vote for leaving things as is with
a comment above the two #include directives.
Inlusion loops are landmines. Evidence: the compilation failure Phil
ran in, leading to his
Subject: [PATCH-for-8.0] coroutine: Add missing <qemu/atomic.h> include
Message-Id:<20221125175532.48858-1-phi...@linaro.org>
Your macro hack I find too off-putting 😄
I think the macro is much better than nerfing qemu/lockable.h.
Another alternative is to add a new header qemu/lockable-protos.h and
move qemu_co_mutex_{lock,unlock} there (possibly other prototypes as
well). Then include it from both qemu/lockable.h and qemu/coroutine.h.
Paolo
