[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/21] qmp: Dumb down how we run QMP command reg
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 04/21] qmp: Dumb down how we run QMP command registration |
Date: |
Fri, 24 Feb 2017 21:21:14 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 02/23/2017 03:45 PM, Markus Armbruster wrote:
>> The way we get QMP commands registered is high tech:
>>
>> * qapi-commands.py generates qmp_init_marshal() that does the actual work
>>
>> * it also generates the magic to register it as a MODULE_INIT_QAPI
>> function, so it runs when someone calls
>> module_call_init(MODULE_INIT_QAPI)
>>
>> * main() calls module_call_init()
>>
>> QEMU needs to register a few non-qapified commands. Same high tech
>> works: monitor.c has its own qmp_init_marshal() along with the magic
>> to make it run in module_call_init(MODULE_INIT_QAPI).
>
> Eww. Two static functions with the same name, which really messes with
> gdb debugging.
>
>>
>> QEMU also needs to unregister commands that are not wanted in this
>> build's configuration (commit 5032a16). Simple enough:
>> qmp_unregister_commands_hack(). The difficulty is to make it run
>> after the generated qmp_init_marshal(). We can't simply run it in
>> monitor.c's qmp_init_marshal(), because the order in which the
>> registered functions run is indeterminate. So qmp_init_marshal()
>> registers qmp_init_marshal() separately. Since registering *appends*
>
> Another case of "A sets up A" when you meant "A sets up B", but this
> time, I'm fairly certain you mean qmp_init_marshal() registers
> qmp_unregister_commands_hack() separately.
Correct. I'll fix it.
>> to the list of registered functions, this will make it run after all
>> the functions that have been registered already.
>>
>> I suspect it takes a long and expensive computer science education to
>> not find this silly.
>
> ROFL
> (does that mean I didn't spend enough on my education? I'm sure you
> could arrange to sell me an online certificate if I wanted more... :)
Plenty of peddlers out there!
>> Dumb it down as follows:
>>
>> * Drop MODULE_INIT_QAPI entirely
>>
>> * Give the generated qmp_init_marshal() external linkage.
>>
>> * Call it instead of module_call_init(MODULE_INIT_QAPI)
>>
>> * Except in QEMU proper, call new monitor_init_qmp_commands() that in
>> turn calls the generated qmp_init_marshal(), registers the
>> additional commands and unregisters the unwanted ones.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>> -static void qmp_init_marshal(void)
>> +void monitor_init_qmp_commands(void)
>> {
>> + qmp_init_marshal();
>> +
>> qmp_register_command("query-qmp-schema", qmp_query_qmp_schema,
>> QCO_NO_OPTIONS);
>> qmp_register_command("device_add", qmp_device_add,
>> @@ -1004,12 +1006,9 @@ static void qmp_init_marshal(void)
>> qmp_register_command("netdev_add", qmp_netdev_add,
>> QCO_NO_OPTIONS);
>>
>> - /* call it after the rest of qapi_init() */
>> - register_module_init(qmp_unregister_commands_hack, MODULE_INIT_QAPI);
>> + qmp_unregister_commands_hack();
>
> We could inline this function now, but keeping the _hack() name around
> reminds us to consider conditional-compilation support in the generator
> at a later date, so I'm fine with the approach you took.
Exactly.
> Reviewed-by: Eric Blake <address@hidden>
Thanks!