Hi
On Thu, Aug 17, 2017 at 11:16 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Add #if defined(CONFIG_REPLICATION) in generated code, and adjust the
>> code accordingly.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> qapi-schema.json | 12 ++++++++----
>> migration/colo.c | 14 ++------------
>> monitor.c | 5 -----
>> 3 files changed, 10 insertions(+), 21 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index bcee3157b0..2f4528c769 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -6337,7 +6337,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:
>> @@ -6352,7 +6353,8 @@
>> # Since: 2.9
>> ##
>> { 'struct': 'ReplicationStatus',
>> - 'data': { 'error': 'bool', '*desc': 'str' } }
>> + 'data': { 'error': 'bool', '*desc': 'str' },
>> + 'if': 'defined(CONFIG_REPLICATION)' }
>>
>> ##
>> # @query-xen-replication-status:
>> @@ -6369,7 +6371,8 @@
>> # Since: 2.9
>> ##
>> { 'command': 'query-xen-replication-status',
>> - 'returns': 'ReplicationStatus' }
>> + 'returns': 'ReplicationStatus',
>> + 'if': 'defined(CONFIG_REPLICATION)' }
>>
>> ##
>> # @xen-colo-do-checkpoint:
>> @@ -6385,7 +6388,8 @@
>> #
>> # Since: 2.9
>> ##
>> -{ 'command': 'xen-colo-do-checkpoint' }
>> +{ 'command': 'xen-colo-do-checkpoint',
>> + 'if': 'defined(CONFIG_REPLICATION)' }
>>
>> ##
>> # @GICCapability:
>> diff --git a/migration/colo.c b/migration/colo.c
>> index a4255432ac..3bff9fc9a4 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -147,11 +147,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;
>> @@ -170,14 +170,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);
>>
>> @@ -192,19 +188,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
>>
>> static void colo_send_message(QEMUFile *f, COLOMessage msg,
>> Error **errp)
>> diff --git a/monitor.c b/monitor.c
>> index 4bf6a3ea2e..383c84d162 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -970,11 +970,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");
>> #endif
>
> Same drill as for PATCH 16.
>
> Commands you make 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 close to killing qmp_unregister_commands_hack().
>
> Check for completeness by reviewing the case insensitive occurrences of
> replication in the schema not covered by your patch, and review uses of
> CONFIG_REPLICATION for possible schema connections (I did that for
> CONFIG_VNC [PATCH 16] and CONFIG_SPICE [PATCH 17], too):
>
> * enum BlockdevDriver value "replication" in command blockdev-add
>
> * Makefile.objs:block-obj-$(CONFIG_REPLICATION) += replication.o
> block/Makefile.objs:block-obj-$(CONFIG_REPLICATION) += replication.o
>
> I think the following should be ifdef CONFIG_REPLICATION: enum
> BlockdevDriver value @replication BlockdevOptions variant @replication,
> BlockdevOptionsReplication, BlockdevOptionsReplicationMode.
Added
>
> Some of this analysis should perhaps be worked into the commit message.
>
Done
thanks
--
Marc-André Lureau