[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v2 13/15] Revert "qapi-events: add 'if' cond
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [RFC PATCH v2 13/15] Revert "qapi-events: add 'if' condition to implicit event enum" |
Date: |
Wed, 19 Dec 2018 09:40:03 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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
>>
>>
- Re: [Qemu-devel] [RFC PATCH v2 01/15] qapi: Belatedly update docs for commit 9c2f56e9f9d, (continued)
- [Qemu-devel] [RFC PATCH v2 04/15] build-sys: move qmp-introspect per target, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 03/15] qapi: Generate QAPIEvent stuff into separate files WIP, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 02/15] qapi: Eliminate indirection through qmp_event_get_func_emit(), Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 12/15] qapi: remove qmp_unregister_command(), Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 13/15] Revert "qapi-events: add 'if' condition to implicit event enum", Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 08/15] target.json: add a note about query-cpu* not being s390x-specific, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 15/15] qapi: move RTC_CHANGE to the target schema, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 11/15] qapi: make query-cpu-definitions depend on specific targets, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 10/15] qapi: make query-cpu-model-expansion depend on s390 or x86, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 05/15] qapi: New module target.json, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 09/15] qapi: make query-gic-capabilities depend on TARGET_ARM, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 06/15] qapi: make rtc-reset-reinjection and SEV depend on TARGET_I386, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 14/15] qmp: Deprecate query-events in favor of query-qmp-schema, Markus Armbruster, 2018/12/18
- [Qemu-devel] [RFC PATCH v2 07/15] qapi: make s390 commands depend on TARGET_S390X, Markus Armbruster, 2018/12/18