Re: [Qemu-devel] [PATCH 04/21] qmp: Dumb down how we run QMP command registration

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>> ---
>
>> -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 <eblake@redhat.com>

Thanks!