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

[Paolo Bonzini]

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-philmd@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