| Message ID | 20181213184340.24037-33-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/32] cutils: Add qemu_strtod() and qemu_strtod_finite() | expand |
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? Thomas
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. thanks for the report, sorry for the regression
Thomas Huth <thuth@redhat.com> writes: > 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 Reproduced. > Any ideas how to fix it? The problem is fairly obvious, repair less so. Sorry this escaped review. Union BlockdevCreateOptions doesn't specify a variant for tag 'replication'. The default variant needs to inherits its condition from the enumeration value. I hope we can develop a fix quickly.
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 :)
On 2018-12-17 20:18, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> 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 > > Reproduced. > >> Any ideas how to fix it? > > The problem is fairly obvious, repair less so. Sorry this escaped > review. This is the second time in 2 months that compilation with --disable-replication broke. So instead of relying on reviews here, I think we should catch this with Patchew / some docker based compilation test instead that configures the build process with everything disabled, e.g.: ./configure --enable-werror --disable-rdma --disable-slirp \ --disable-curl --disable-capstone --disable-live-block-migration \ --disable-glusterfs --disable-replication --disable-coroutine-pool \ --disable-smartcard --disable-guest-agent --disable-curses \ --disable-libxml2 --disable-tpm --disable-qom-cast-debug \ --disable-spice --disable-vhost-vsock --disable-vhost-net \ --disable-vhost-crypto --disable-vhost-user ... (I'm still one of those docker agnostics ... so if someone wants to come up with a patch, I'd be very glad ... otherwise it'll take some time 'til I figured it out how to add a docker based test for this...) Thanks, Thomas
Thomas Huth <thuth@redhat.com> writes: > This is the second time in 2 months that compilation with > --disable-replication broke. So instead of relying on reviews here, I > think we should catch this with Patchew / some docker based compilation > test instead that configures the build process with everything disabled, > e.g.: > > ./configure --enable-werror --disable-rdma --disable-slirp \ > --disable-curl --disable-capstone --disable-live-block-migration \ > --disable-glusterfs --disable-replication --disable-coroutine-pool \ > --disable-smartcard --disable-guest-agent --disable-curses \ > --disable-libxml2 --disable-tpm --disable-qom-cast-debug \ > --disable-spice --disable-vhost-vsock --disable-vhost-net \ > --disable-vhost-crypto --disable-vhost-user ... > > (I'm still one of those docker agnostics ... so if someone wants to come > up with a patch, I'd be very glad ... otherwise it'll take some time > 'til I figured it out how to add a docker based test for this...) In my limited experience, a "disable everything" build requires more frequent tinkering than I'd like. A --disable-all would be nice for this, but I can't spare the time right now. Any takers?
diff --git a/migration/colo.c b/migration/colo.c index fcff04c78c..398b239d1c 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -24,7 +24,9 @@ #include "trace.h" #include "qemu/error-report.h" #include "migration/failover.h" +#ifdef CONFIG_REPLICATION #include "replication.h" +#endif #include "net/colo-compare.h" #include "net/colo.h" #include "block/block.h" @@ -201,11 +203,11 @@ void colo_do_failover(MigrationState *s) } } +#ifdef CONFIG_REPLICATION void qmp_xen_set_replication(bool enable, bool primary, bool has_failover, bool failover, Error **errp) { -#ifdef CONFIG_REPLICATION ReplicationMode mode = primary ? REPLICATION_MODE_PRIMARY : REPLICATION_MODE_SECONDARY; @@ -224,14 +226,10 @@ void qmp_xen_set_replication(bool enable, bool primary, } replication_stop_all(failover, failover ? NULL : errp); } -#else - abort(); -#endif } ReplicationStatus *qmp_query_xen_replication_status(Error **errp) { -#ifdef CONFIG_REPLICATION Error *err = NULL; ReplicationStatus *s = g_new0(ReplicationStatus, 1); @@ -246,19 +244,13 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp) error_free(err); return s; -#else - abort(); -#endif } void qmp_xen_colo_do_checkpoint(Error **errp) { -#ifdef CONFIG_REPLICATION replication_do_checkpoint_all(errp); -#else - abort(); -#endif } +#endif COLOStatus *qmp_query_colo_status(Error **errp) { diff --git a/monitor.c b/monitor.c index cf9cece965..7848a3cb0d 100644 --- a/monitor.c +++ b/monitor.c @@ -1147,11 +1147,6 @@ static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data, */ static void qmp_unregister_commands_hack(void) { -#ifndef CONFIG_REPLICATION - qmp_unregister_command(&qmp_commands, "xen-set-replication"); - qmp_unregister_command(&qmp_commands, "query-xen-replication-status"); - qmp_unregister_command(&qmp_commands, "xen-colo-do-checkpoint"); -#endif #ifndef TARGET_I386 qmp_unregister_command(&qmp_commands, "rtc-reset-reinjection"); qmp_unregister_command(&qmp_commands, "query-sev"); diff --git a/qapi/block-core.json b/qapi/block-core.json index 92e0205d91..762000f31f 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2623,7 +2623,9 @@ 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow', - 'qcow2', 'qed', 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', + 'qcow2', 'qed', 'quorum', 'raw', 'rbd', + { 'name': 'replication', 'if': 'defined(CONFIG_REPLICATION)' }, + 'sheepdog', 'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] } ## @@ -3380,7 +3382,8 @@ # # Since: 2.9 ## -{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ] } +{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ], + 'if': 'defined(CONFIG_REPLICATION)' } ## # @BlockdevOptionsReplication: @@ -3398,7 +3401,8 @@ { 'struct': 'BlockdevOptionsReplication', 'base': 'BlockdevOptionsGenericFormat', 'data': { 'mode': 'ReplicationMode', - '*top-id': 'str' } } + '*top-id': 'str' }, + 'if': 'defined(CONFIG_REPLICATION)' } ## # @NFSTransport: @@ -3714,7 +3718,8 @@ 'quorum': 'BlockdevOptionsQuorum', 'raw': 'BlockdevOptionsRaw', 'rbd': 'BlockdevOptionsRbd', - 'replication':'BlockdevOptionsReplication', + 'replication': { 'type': 'BlockdevOptionsReplication', + 'if': 'defined(CONFIG_REPLICATION)' }, 'sheepdog': 'BlockdevOptionsSheepdog', 'ssh': 'BlockdevOptionsSsh', 'throttle': 'BlockdevOptionsThrottle', diff --git a/qapi/migration.json b/qapi/migration.json index 5fd33316a0..31b589ec26 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1257,7 +1257,8 @@ # Since: 2.9 ## { 'command': 'xen-set-replication', - 'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } } + 'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' }, + 'if': 'defined(CONFIG_REPLICATION)' } ## # @ReplicationStatus: @@ -1272,7 +1273,8 @@ # Since: 2.9 ## { 'struct': 'ReplicationStatus', - 'data': { 'error': 'bool', '*desc': 'str' } } + 'data': { 'error': 'bool', '*desc': 'str' }, + 'if': 'defined(CONFIG_REPLICATION)' } ## # @query-xen-replication-status: @@ -1289,7 +1291,8 @@ # Since: 2.9 ## { 'command': 'query-xen-replication-status', - 'returns': 'ReplicationStatus' } + 'returns': 'ReplicationStatus', + 'if': 'defined(CONFIG_REPLICATION)' } ## # @xen-colo-do-checkpoint: @@ -1305,7 +1308,8 @@ # # Since: 2.9 ## -{ 'command': 'xen-colo-do-checkpoint' } +{ 'command': 'xen-colo-do-checkpoint', + 'if': 'defined(CONFIG_REPLICATION)' } ## # @COLOStatus: