Re: [PATCH 4/4] coroutine: Break inclusion loop

[Philippe Mathieu-Daudé]

On 13/12/22 01:34, Paolo Bonzini wrote:
>     On 12/8/22 15:23, Markus Armbruster wrote:
>      > qemu/coroutine.h and qemu/lockable.h include each other.  Neither
>      > header actually needs the other one.
>
>     qemu/lockable.h wants qemu/coroutine.h because of the reference to
>     qemu_co_mutex_lock/unlock in the QEMU_MAKE_LOCKABLE macro.  Said
>     reference only happens when the macro is used, so strictly speaking
>     only code that uses of qemu/lockable.h's functionality needs to
>     include qemu/coroutine.h.  The order doesn't matter.

[*]

>     qemu/coroutine.h similarly wants qemu/lockable.h only for a macro: it
>     uses QemuLockable for the prototype of qemu_co_queue_wait_impl, but
>     QemuLockable is defined in qemu/typedefs.h.  On the other hand, the
>     qemu_co_queue_wait macro needs QEMU_MAKE_LOCKABLE.  Again, the order
>     does not matter but callers of qemu_co_queue_wait appreciate it if
>     both files are included.
>
>     So, this is why the inclusion loop works.  This patch makes some
>     files include qemu/coroutine.h even if they only need qemu/lockable.h
>     for QEMU_LOCK_GUARD of a "regular" QemuMutex; for example, linux-user/
>     does not use coroutines, so I'd like to avoid that it includes
>     qemu/coroutine.h.
>
>     One way is to just keep the cycle.  Another is to break the cycle is
>     as follows:
>
>     1) qemu/coroutine.h keeps including qemu/lockable.h
>
>     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)                       
>           \

Interesting, I ended doing something similar today because this line is
the single sysemu-specific part of this file (user emulation shouldn't
have access to qemu/coroutine.h). So back to [*], the order seems to
matter for user-mode.

>                     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)),
>
>
>     Neither is particularly pretty, so I vote for leaving things as is with
>     a comment above the two #include directives.
>
>     Paolo