Re: [Qemu-devel] [PULL 32/32] qapi: add conditions to REPLICATION type/commands on the schema

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Dec 17, 2018 at 8:01 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 2018-12-13 19:43, Markus Armbruster wrote:
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Add #if defined(CONFIG_REPLICATION) in generated code, and adjust the
>> > code accordingly.
>> >
>> > Made conditional:
>> >
>> > * xen-set-replication, query-xen-replication-status,
>> >   xen-colo-do-checkpoint
>> >
>> >   Before the patch, we first register the commands unconditionally in
>> >   generated code (requires a stub), then conditionally unregister in
>> >   qmp_unregister_commands_hack().
>> >
>> >   Afterwards, we register only when CONFIG_REPLICATION.  The command
>> >   fails exactly the same, with CommandNotFound.
>> >
>> >   Improvement, because now query-qmp-schema is accurate, and we're one
>> >   step closer to killing qmp_unregister_commands_hack().
>> >
>> > * enum BlockdevDriver value "replication" in command blockdev-add
>> >
>> > * BlockdevOptions variant @replication
>> >
>> > and related structures.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> > Message-Id: <20181213123724.4866-23-marcandre.lureau@redhat.com>
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  migration/colo.c     | 16 ++++------------
>> >  monitor.c            |  5 -----
>> >  qapi/block-core.json | 13 +++++++++----
>> >  qapi/migration.json  | 12 ++++++++----
>> >  4 files changed, 21 insertions(+), 25 deletions(-)
>>
>> I think this might have broken compilation with --disable-replication:
>>
>>  https://gitlab.com/huth/qemu/-/jobs/135648240
>>
>> Any ideas how to fix it?
>
> We introduced a regression by dropping osdep.h include from headers.
>
> To me, it's best to include osdep.h in headers, since the ifdef exist
> there. But I have been told we include it in .c instead. I'll upstream
> the qapi/scripts to include it.

This can't be the cause: qapi/qapi-visit-block-core.c includes osdep.h
first, like all our .c files.

> thanks for the report, sorry for the regression

You've since posted a fix to add the missing ifdeffery.  I'm testing it
while I write :)