Re: [Qemu-devel] [RFC PATCH v2 13/15] Revert "qapi-events: add 'if' condition to implicit event enum"

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Tue, Dec 18, 2018 at 10:27 PM Markus Armbruster <address@hidden> wrote:
>>
>> This reverts commit 7bd263490590ee6fcf34ecb6203437e22f6e5a9c.
>>
>> The commit applied the events' conditions to the members of enum
>> QAPIEvent.  Awkward, because it renders QAPIEvent unusable in
>> target-independent code as soon as we make an event target-dependent.
>> Reverting this has the following effects:
>>
>> * ui/vnc.c can remain target independent.
>
> Was it ever moved? I don't recall

It's currently target-independent, and we want it to remain that way.

You keep it that way by splitting target-dependent target_QAPIEvent off
QAPIEvent.

I keep it that way by not making any enum members target-dependent.

>> * monitor_qapi_event_conf[] doesn't have to muck around with #ifdef.
>
> I suggested a way to get rid of monitor_qapi_event_conf[] in the patch
> message, by having the rate stored in the schema, which could actually
> be useful (for doc, introspection etc).

Introspection and generated documentation improvements might still make
that worthwhile.  For the former, we'd first want an actual user,
though.

>> * query-events again doesn't reflect conditionals.  I'm going to
>>   deprecate it in favor of query-qmp-schema.
>
> I guess that's not that important.

query-qmp-schema has reflected conditionals since we introduced them in
v3.0.  query-events has not.  Your commit fixes it for 4.0.  Fixes are
good, but when an interface is known to be deficient in some versions,
while a strictly more powerful buddy is fine in all versions, the
obvious thing to do is to stay away from the deficient one regardless of
version.

I think we should deprecate query-events even if we decide to fix it
now.

I'll work this into the next commit's commit message.

> I have a slight preference for not declaring enums when the related
> option is disabled.

Me too, but I like keeping things simple even more.

Possibly even simpler: dispense with the enumeration type, add suitable
#define to each qapi-event-MODULE.h.  We can have #ifdef there.  What do
you think?

> But people don't like having too much #ifdef code, which is understandable.

In this case, it's just one big #if in monitor.c.  Tolerable, I think.

If we replace QAPIEvent by #define, the #if shrinks to just #ifdef
QAPI_EVENT_RTC_CHANGE.

Note that monitor.c is already target-dependent.  It should really be
split up, though.  The #if would keep the QMP part target-dependent,
unless we split it up even more.  Hmm.

>> Signed-off-by: Markus Armbruster <address@hidden>
>>
>> # Conflicts:
>> #       scripts/qapi/events.py
>> ---
>>  scripts/qapi/events.py | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index e988e43941..c944ba90b8 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -194,7 +194,9 @@ void %(event_emit)s(%(event_enum)s event, QDict *qdict);
>>              self._genc.add(gen_event_send(name, arg_type, boxed,
>>                                            self._event_enum_name,
>>                                            self._event_emit_name))
>> -        self._event_enum_members.append(QAPISchemaMember(name, ifcond))
>> +        # Note: we generate the enum member regardless of @ifcond, to
>> +        # keep the enumeration usable in target-independent code.
>> +        self._event_enum_members.append(QAPISchemaMember(name))
>>
>>
>>  def gen_events(schema, output_dir, prefix):
>> --
>> 2.17.2
>>
>>