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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> dropped qemu-devel by mistake.
>
> Paolo
>
>
> Il lun 12 dic 2022, 23:16 Paolo Bonzini <pbonzini@redhat.com> ha scritto:
>
>> 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.

They include it even before the patch, via lockable.h.

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?

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

As in my patch.

>> 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.

>> 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 :)

>>> Drop #include "qemu/coroutine.h" from qemu/lockable.h to break the
>>> loop.  All users of lockable.h actually need qemu/coroutine.h, so
>>> adjust their #include directives.
>>> 
>>> I'm not dropping the #include "qemu/lockable.h" from qemu/coroutine.h,
>>> because that would require adding it back next to #include
>>> "qemu/coroutine.h" all over the place.  It's in an unusual place,
>>> though.  Move it to the usual place, next to the other #include
>>> directives.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>