Don't compile-in replication-related files when replication is disabled
in config.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

Hi all!

I'm unsure, should it be actually separate
--disable-colo / --enable-colo options or it's really used only together
with replication staff.. So, I decided to start with simpler variant.


 block/meson.build     |  2 +-
 migration/meson.build |  6 ++++--
 net/meson.build       |  8 ++++----
 qapi/migration.json   |  6 ++++--
 stubs/colo.c          | 46 +++++++++++++++++++++++++++++++++++++++++++
 stubs/meson.build     |  1 +
 6 files changed, 60 insertions(+), 9 deletions(-)
 create mode 100644 stubs/colo.c

diff --git a/block/meson.build b/block/meson.build
index 382bec0e7d..b9a72e219b 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c')
 block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
 block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
 block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c'))
-if not get_option('replication').disabled()
+if get_option('replication').allowed()
   block_ss.add(files('replication.c'))
 endif
 block_ss.add(when: libaio, if_true: files('linux-aio.c'))
diff --git a/migration/meson.build b/migration/meson.build
index 0d1bb9f96e..8180eaea7b 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -13,8 +13,6 @@ softmmu_ss.add(files(
   'block-dirty-bitmap.c',
   'channel.c',
   'channel-block.c',
-  'colo-failover.c',
-  'colo.c',
   'exec.c',
   'fd.c',
   'global_state.c',
@@ -29,6 +27,10 @@ softmmu_ss.add(files(
   'threadinfo.c',
 ), gnutls)
 
+if get_option('replication').allowed()
+  softmmu_ss.add(files('colo.c', 'colo-failover.c'))
+endif
+
 softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
 if get_option('live_block_migration').allowed()
   softmmu_ss.add(files('block.c'))
diff --git a/net/meson.build b/net/meson.build
index 87afca3e93..634ab71cc6 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -1,13 +1,9 @@
 softmmu_ss.add(files(
   'announce.c',
   'checksum.c',
-  'colo-compare.c',
-  'colo.c',
   'dump.c',
   'eth.c',
   'filter-buffer.c',
-  'filter-mirror.c',
-  'filter-rewriter.c',
   'filter.c',
   'hub.c',
   'net-hmp-cmds.c',
@@ -19,6 +15,10 @@ softmmu_ss.add(files(
   'util.c',
 ))
 
+if get_option('replication').allowed()
+  softmmu_ss.add(files('colo-compare.c', 'colo.c', 'filter-rewriter.c', 'filter-mirror.c'))
+endif
+
 softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
 
 if have_l2tpv3
diff --git a/qapi/migration.json b/qapi/migration.json
index c84fa10e86..5b81e09369 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1685,7 +1685,8 @@
 ##
 { 'struct': 'COLOStatus',
   'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
-            'reason': 'COLOExitReason' } }
+            'reason': 'COLOExitReason' },
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @query-colo-status:
@@ -1702,7 +1703,8 @@
 # Since: 3.1
 ##
 { 'command': 'query-colo-status',
-  'returns': 'COLOStatus' }
+  'returns': 'COLOStatus',
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @migrate-recover:
diff --git a/stubs/colo.c b/stubs/colo.c
new file mode 100644
index 0000000000..5a02540baa
--- /dev/null
+++ b/stubs/colo.c
@@ -0,0 +1,46 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+#include "migration/colo.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+
+void colo_compare_cleanup(void)
+{
+    abort();
+}
+
+void colo_shutdown(void)
+{
+    abort();
+}
+
+void *colo_process_incoming_thread(void *opaque)
+{
+    abort();
+}
+
+void colo_checkpoint_notify(void *opaque)
+{
+    abort();
+}
+
+void migrate_start_colo_process(MigrationState *s)
+{
+    abort();
+}
+
+bool migration_in_colo_state(void)
+{
+    return false;
+}
+
+bool migration_incoming_in_colo_state(void)
+{
+    return false;
+}
+
+void qmp_x_colo_lost_heartbeat(Error **errp)
+{
+    error_setg(errp, "COLO support is not built in");
+}
diff --git a/stubs/meson.build b/stubs/meson.build
index b2b5956d97..8412cad15f 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
 stub_ss.add(files('target-monitor-defs.c'))
 stub_ss.add(files('trace-control.c'))
 stub_ss.add(files('uuid.c'))
+stub_ss.add(files('colo.c'))
 stub_ss.add(files('vmstate.c'))
 stub_ss.add(files('vm-stop.c'))
 stub_ss.add(files('win32-kbd-hook.c'))
-- 
2.34.1

On 11.04.23 17:51, Vladimir Sementsov-Ogievskiy wrote:
> Don't compile-in replication-related files when replication is disabled
> in config.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>

v2 will be called "[PATCH v2 0/4] COLO: improve build options"

-- 
Best regards,
Vladimir

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Tuesday, April 11, 2023 10:51 PM
> To: qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; pbonzini@redhat.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; Zhang, Chen
> <chen.zhang@intel.com>; lizhijian@fujitsu.com;
> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
> plotnikov@yandex-team.ru; Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru>
> Subject: [PATCH] replication: compile out some staff when replication is not
> configured
> 
> Don't compile-in replication-related files when replication is disabled in
> config.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> Hi all!
> 
> I'm unsure, should it be actually separate --disable-colo / --enable-colo
> options or it's really used only together with replication staff.. So, I decided
> to start with simpler variant.
> 

For replication, I think there's nothing wrong with the idea.
But not so for COLO.  COLO project consists of three independent parts: Replication, migration, net-proxy.
Each one have ability to run alone for other proposals. For example we can just run filter-mirror/redirector for networking
Analysis/debugs. Although the best practice of COLO is to make the three modules work together, in fact, we can also
use only some modules of COLO for other usage scenarios. Like COLO migration + net-proxy for shared disk, etc...
So I think no need to disable all COLO related modules when replication is not configured.
For details:
https://wiki.qemu.org/Features/COLO

Thanks
Chen

> 
>  block/meson.build     |  2 +-
>  migration/meson.build |  6 ++++--
>  net/meson.build       |  8 ++++----
>  qapi/migration.json   |  6 ++++--
>  stubs/colo.c          | 46 +++++++++++++++++++++++++++++++++++++++++++
>  stubs/meson.build     |  1 +
>  6 files changed, 60 insertions(+), 9 deletions(-)  create mode 100644
> stubs/colo.c
> 
> diff --git a/block/meson.build b/block/meson.build index
> 382bec0e7d..b9a72e219b 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-
> win32.c', 'win32-aio.c')
>  block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
>  block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
>  block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c')) -if not
> get_option('replication').disabled()
> +if get_option('replication').allowed()
>    block_ss.add(files('replication.c'))
>  endif
>  block_ss.add(when: libaio, if_true: files('linux-aio.c')) diff --git
> a/migration/meson.build b/migration/meson.build index
> 0d1bb9f96e..8180eaea7b 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -13,8 +13,6 @@ softmmu_ss.add(files(
>    'block-dirty-bitmap.c',
>    'channel.c',
>    'channel-block.c',
> -  'colo-failover.c',
> -  'colo.c',
>    'exec.c',
>    'fd.c',
>    'global_state.c',
> @@ -29,6 +27,10 @@ softmmu_ss.add(files(
>    'threadinfo.c',
>  ), gnutls)
> 
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo.c', 'colo-failover.c')) endif
> +
>  softmmu_ss.add(when: rdma, if_true: files('rdma.c'))  if
> get_option('live_block_migration').allowed()
>    softmmu_ss.add(files('block.c'))
> diff --git a/net/meson.build b/net/meson.build index
> 87afca3e93..634ab71cc6 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -1,13 +1,9 @@
>  softmmu_ss.add(files(
>    'announce.c',
>    'checksum.c',
> -  'colo-compare.c',
> -  'colo.c',
>    'dump.c',
>    'eth.c',
>    'filter-buffer.c',
> -  'filter-mirror.c',
> -  'filter-rewriter.c',
>    'filter.c',
>    'hub.c',
>    'net-hmp-cmds.c',
> @@ -19,6 +15,10 @@ softmmu_ss.add(files(
>    'util.c',
>  ))
> 
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo-compare.c', 'colo.c', 'filter-rewriter.c',
> +'filter-mirror.c')) endif
> +
>  softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
> 
>  if have_l2tpv3
> diff --git a/qapi/migration.json b/qapi/migration.json index
> c84fa10e86..5b81e09369 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1685,7 +1685,8 @@
>  ##
>  { 'struct': 'COLOStatus',
>    'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> -            'reason': 'COLOExitReason' } }
> +            'reason': 'COLOExitReason' },
> +  'if': 'CONFIG_REPLICATION' }
> 
>  ##
>  # @query-colo-status:
> @@ -1702,7 +1703,8 @@
>  # Since: 3.1
>  ##
>  { 'command': 'query-colo-status',
> -  'returns': 'COLOStatus' }
> +  'returns': 'COLOStatus',
> +  'if': 'CONFIG_REPLICATION' }
> 
>  ##
>  # @migrate-recover:
> diff --git a/stubs/colo.c b/stubs/colo.c new file mode 100644 index
> 0000000000..5a02540baa
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,46 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_compare_cleanup(void)
> +{
> +    abort();
> +}
> +
> +void colo_shutdown(void)
> +{
> +    abort();
> +}
> +
> +void *colo_process_incoming_thread(void *opaque) {
> +    abort();
> +}
> +
> +void colo_checkpoint_notify(void *opaque) {
> +    abort();
> +}
> +
> +void migrate_start_colo_process(MigrationState *s) {
> +    abort();
> +}
> +
> +bool migration_in_colo_state(void)
> +{
> +    return false;
> +}
> +
> +bool migration_incoming_in_colo_state(void)
> +{
> +    return false;
> +}
> +
> +void qmp_x_colo_lost_heartbeat(Error **errp) {
> +    error_setg(errp, "COLO support is not built in"); }
> diff --git a/stubs/meson.build b/stubs/meson.build index
> b2b5956d97..8412cad15f 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
>  stub_ss.add(files('target-monitor-defs.c'))
>  stub_ss.add(files('trace-control.c'))
>  stub_ss.add(files('uuid.c'))
> +stub_ss.add(files('colo.c'))
>  stub_ss.add(files('vmstate.c'))
>  stub_ss.add(files('vm-stop.c'))
>  stub_ss.add(files('win32-kbd-hook.c'))
> --
> 2.34.1

On 13.04.23 12:52, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Tuesday, April 11, 2023 10:51 PM
>> To: qemu-devel@nongnu.org
>> Cc: qemu-block@nongnu.org; pbonzini@redhat.com; armbru@redhat.com;
>> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
>> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
>> Hailiang <zhanghailiang@xfusion.com>; Zhang, Chen
>> <chen.zhang@intel.com>; lizhijian@fujitsu.com;
>> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
>> plotnikov@yandex-team.ru; Vladimir Sementsov-Ogievskiy
>> <vsementsov@yandex-team.ru>
>> Subject: [PATCH] replication: compile out some staff when replication is not
>> configured
>>
>> Don't compile-in replication-related files when replication is disabled in
>> config.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>
>> Hi all!
>>
>> I'm unsure, should it be actually separate --disable-colo / --enable-colo
>> options or it's really used only together with replication staff.. So, I decided
>> to start with simpler variant.
>>
> 
> For replication, I think there's nothing wrong with the idea.
> But not so for COLO.  COLO project consists of three independent parts: Replication, migration, net-proxy.
> Each one have ability to run alone for other proposals. For example we can just run filter-mirror/redirector for networking
> Analysis/debugs. Although the best practice of COLO is to make the three modules work together, in fact, we can also
> use only some modules of COLO for other usage scenarios. Like COLO migration + net-proxy for shared disk, etc...
> So I think no need to disable all COLO related modules when replication is not configured.
> For details:
> https://wiki.qemu.org/Features/COLO
> 

So, if I want to have an option to disable all COLO modules, do you mean it should be additional --disable-colo option? Or better keep one option --disable-replication (and, maybe just rename to to --disable-colo)?

> Thanks
> Chen
> 
>>
>>   block/meson.build     |  2 +-
>>   migration/meson.build |  6 ++++--
>>   net/meson.build       |  8 ++++----
>>   qapi/migration.json   |  6 ++++--
>>   stubs/colo.c          | 46 +++++++++++++++++++++++++++++++++++++++++++
>>   stubs/meson.build     |  1 +
>>   6 files changed, 60 insertions(+), 9 deletions(-)  create mode 100644
>> stubs/colo.c
>>
>> diff --git a/block/meson.build b/block/meson.build index
>> 382bec0e7d..b9a72e219b 100644
>> --- a/block/meson.build
>> +++ b/block/meson.build
>> @@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-
>> win32.c', 'win32-aio.c')
>>   block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
>>   block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
>>   block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c')) -if not
>> get_option('replication').disabled()
>> +if get_option('replication').allowed()
>>     block_ss.add(files('replication.c'))
>>   endif
>>   block_ss.add(when: libaio, if_true: files('linux-aio.c')) diff --git
>> a/migration/meson.build b/migration/meson.build index
>> 0d1bb9f96e..8180eaea7b 100644
>> --- a/migration/meson.build
>> +++ b/migration/meson.build
>> @@ -13,8 +13,6 @@ softmmu_ss.add(files(
>>     'block-dirty-bitmap.c',
>>     'channel.c',
>>     'channel-block.c',
>> -  'colo-failover.c',
>> -  'colo.c',
>>     'exec.c',
>>     'fd.c',
>>     'global_state.c',
>> @@ -29,6 +27,10 @@ softmmu_ss.add(files(
>>     'threadinfo.c',
>>   ), gnutls)
>>
>> +if get_option('replication').allowed()
>> +  softmmu_ss.add(files('colo.c', 'colo-failover.c')) endif
>> +
>>   softmmu_ss.add(when: rdma, if_true: files('rdma.c'))  if
>> get_option('live_block_migration').allowed()
>>     softmmu_ss.add(files('block.c'))
>> diff --git a/net/meson.build b/net/meson.build index
>> 87afca3e93..634ab71cc6 100644
>> --- a/net/meson.build
>> +++ b/net/meson.build
>> @@ -1,13 +1,9 @@
>>   softmmu_ss.add(files(
>>     'announce.c',
>>     'checksum.c',
>> -  'colo-compare.c',
>> -  'colo.c',
>>     'dump.c',
>>     'eth.c',
>>     'filter-buffer.c',
>> -  'filter-mirror.c',
>> -  'filter-rewriter.c',
>>     'filter.c',
>>     'hub.c',
>>     'net-hmp-cmds.c',
>> @@ -19,6 +15,10 @@ softmmu_ss.add(files(
>>     'util.c',
>>   ))
>>
>> +if get_option('replication').allowed()
>> +  softmmu_ss.add(files('colo-compare.c', 'colo.c', 'filter-rewriter.c',
>> +'filter-mirror.c')) endif
>> +
>>   softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>>
>>   if have_l2tpv3
>> diff --git a/qapi/migration.json b/qapi/migration.json index
>> c84fa10e86..5b81e09369 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1685,7 +1685,8 @@
>>   ##
>>   { 'struct': 'COLOStatus',
>>     'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
>> -            'reason': 'COLOExitReason' } }
>> +            'reason': 'COLOExitReason' },
>> +  'if': 'CONFIG_REPLICATION' }
>>
>>   ##
>>   # @query-colo-status:
>> @@ -1702,7 +1703,8 @@
>>   # Since: 3.1
>>   ##
>>   { 'command': 'query-colo-status',
>> -  'returns': 'COLOStatus' }
>> +  'returns': 'COLOStatus',
>> +  'if': 'CONFIG_REPLICATION' }
>>
>>   ##
>>   # @migrate-recover:
>> diff --git a/stubs/colo.c b/stubs/colo.c new file mode 100644 index
>> 0000000000..5a02540baa
>> --- /dev/null
>> +++ b/stubs/colo.c
>> @@ -0,0 +1,46 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu/notify.h"
>> +#include "net/colo-compare.h"
>> +#include "migration/colo.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qapi-commands-migration.h"
>> +
>> +void colo_compare_cleanup(void)
>> +{
>> +    abort();
>> +}
>> +
>> +void colo_shutdown(void)
>> +{
>> +    abort();
>> +}
>> +
>> +void *colo_process_incoming_thread(void *opaque) {
>> +    abort();
>> +}
>> +
>> +void colo_checkpoint_notify(void *opaque) {
>> +    abort();
>> +}
>> +
>> +void migrate_start_colo_process(MigrationState *s) {
>> +    abort();
>> +}
>> +
>> +bool migration_in_colo_state(void)
>> +{
>> +    return false;
>> +}
>> +
>> +bool migration_incoming_in_colo_state(void)
>> +{
>> +    return false;
>> +}
>> +
>> +void qmp_x_colo_lost_heartbeat(Error **errp) {
>> +    error_setg(errp, "COLO support is not built in"); }
>> diff --git a/stubs/meson.build b/stubs/meson.build index
>> b2b5956d97..8412cad15f 100644
>> --- a/stubs/meson.build
>> +++ b/stubs/meson.build
>> @@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
>>   stub_ss.add(files('target-monitor-defs.c'))
>>   stub_ss.add(files('trace-control.c'))
>>   stub_ss.add(files('uuid.c'))
>> +stub_ss.add(files('colo.c'))
>>   stub_ss.add(files('vmstate.c'))
>>   stub_ss.add(files('vm-stop.c'))
>>   stub_ss.add(files('win32-kbd-hook.c'))
>> --
>> 2.34.1
> 

-- 
Best regards,
Vladimir

> -----Original Message-----
> From: qemu-devel-bounces+chen.zhang=intel.com@nongnu.org <qemu-
> devel-bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Vladimir
> Sementsov-Ogievskiy
> Sent: Thursday, April 13, 2023 9:47 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; pbonzini@redhat.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; lizhijian@fujitsu.com;
> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
> plotnikov@yandex-team.ru
> Subject: Re: [PATCH] replication: compile out some staff when replication is
> not configured
> 
> On 13.04.23 12:52, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> Sent: Tuesday, April 11, 2023 10:51 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: qemu-block@nongnu.org; pbonzini@redhat.com;
> armbru@redhat.com;
> >> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
> >> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
> >> Hailiang <zhanghailiang@xfusion.com>; Zhang, Chen
> >> <chen.zhang@intel.com>; lizhijian@fujitsu.com;
> >> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
> >> plotnikov@yandex-team.ru; Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex-team.ru>
> >> Subject: [PATCH] replication: compile out some staff when replication
> >> is not configured
> >>
> >> Don't compile-in replication-related files when replication is
> >> disabled in config.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex-team.ru>
> >> ---
> >>
> >> Hi all!
> >>
> >> I'm unsure, should it be actually separate --disable-colo /
> >> --enable-colo options or it's really used only together with
> >> replication staff.. So, I decided to start with simpler variant.
> >>
> >
> > For replication, I think there's nothing wrong with the idea.
> > But not so for COLO.  COLO project consists of three independent parts:
> Replication, migration, net-proxy.
> > Each one have ability to run alone for other proposals. For example we
> > can just run filter-mirror/redirector for networking Analysis/debugs.
> > Although the best practice of COLO is to make the three modules work
> together, in fact, we can also use only some modules of COLO for other
> usage scenarios. Like COLO migration + net-proxy for shared disk, etc...
> > So I think no need to disable all COLO related modules when replication is
> not configured.
> > For details:
> > https://wiki.qemu.org/Features/COLO
> >
> 
> So, if I want to have an option to disable all COLO modules, do you mean it
> should be additional --disable-colo option? Or better keep one option --
> disable-replication (and, maybe just rename to to --disable-colo)?

I think keep the option --disable-replication is enough.
Generally speaking, these three modules do not belong to COLO, It has been decoupled at the time of design.
Instead, COLO is formed when these three modules are used in combination.

Thanks
Chen

> 
> > Thanks
> > Chen
> >
> >>
> >>   block/meson.build     |  2 +-
> >>   migration/meson.build |  6 ++++--
> >>   net/meson.build       |  8 ++++----
> >>   qapi/migration.json   |  6 ++++--
> >>   stubs/colo.c          | 46
> +++++++++++++++++++++++++++++++++++++++++++
> >>   stubs/meson.build     |  1 +
> >>   6 files changed, 60 insertions(+), 9 deletions(-)  create mode
> >> 100644 stubs/colo.c
> >>
> >> diff --git a/block/meson.build b/block/meson.build index
> >> 382bec0e7d..b9a72e219b 100644
> >> --- a/block/meson.build
> >> +++ b/block/meson.build
> >> @@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true:
> >> files('file- win32.c', 'win32-aio.c')
> >>   block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref,
> iokit])
> >>   block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
> >>   block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c')) -if
> >> not
> >> get_option('replication').disabled()
> >> +if get_option('replication').allowed()
> >>     block_ss.add(files('replication.c'))
> >>   endif
> >>   block_ss.add(when: libaio, if_true: files('linux-aio.c')) diff
> >> --git a/migration/meson.build b/migration/meson.build index
> >> 0d1bb9f96e..8180eaea7b 100644
> >> --- a/migration/meson.build
> >> +++ b/migration/meson.build
> >> @@ -13,8 +13,6 @@ softmmu_ss.add(files(
> >>     'block-dirty-bitmap.c',
> >>     'channel.c',
> >>     'channel-block.c',
> >> -  'colo-failover.c',
> >> -  'colo.c',
> >>     'exec.c',
> >>     'fd.c',
> >>     'global_state.c',
> >> @@ -29,6 +27,10 @@ softmmu_ss.add(files(
> >>     'threadinfo.c',
> >>   ), gnutls)
> >>
> >> +if get_option('replication').allowed()
> >> +  softmmu_ss.add(files('colo.c', 'colo-failover.c')) endif
> >> +
> >>   softmmu_ss.add(when: rdma, if_true: files('rdma.c'))  if
> >> get_option('live_block_migration').allowed()
> >>     softmmu_ss.add(files('block.c'))
> >> diff --git a/net/meson.build b/net/meson.build index
> >> 87afca3e93..634ab71cc6 100644
> >> --- a/net/meson.build
> >> +++ b/net/meson.build
> >> @@ -1,13 +1,9 @@
> >>   softmmu_ss.add(files(
> >>     'announce.c',
> >>     'checksum.c',
> >> -  'colo-compare.c',
> >> -  'colo.c',
> >>     'dump.c',
> >>     'eth.c',
> >>     'filter-buffer.c',
> >> -  'filter-mirror.c',
> >> -  'filter-rewriter.c',
> >>     'filter.c',
> >>     'hub.c',
> >>     'net-hmp-cmds.c',
> >> @@ -19,6 +15,10 @@ softmmu_ss.add(files(
> >>     'util.c',
> >>   ))
> >>
> >> +if get_option('replication').allowed()
> >> +  softmmu_ss.add(files('colo-compare.c', 'colo.c',
> >> +'filter-rewriter.c',
> >> +'filter-mirror.c')) endif
> >> +
> >>   softmmu_ss.add(when: 'CONFIG_TCG', if_true:
> >> files('filter-replay.c'))
> >>
> >>   if have_l2tpv3
> >> diff --git a/qapi/migration.json b/qapi/migration.json index
> >> c84fa10e86..5b81e09369 100644
> >> --- a/qapi/migration.json
> >> +++ b/qapi/migration.json
> >> @@ -1685,7 +1685,8 @@
> >>   ##
> >>   { 'struct': 'COLOStatus',
> >>     'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> >> -            'reason': 'COLOExitReason' } }
> >> +            'reason': 'COLOExitReason' },
> >> +  'if': 'CONFIG_REPLICATION' }
> >>
> >>   ##
> >>   # @query-colo-status:
> >> @@ -1702,7 +1703,8 @@
> >>   # Since: 3.1
> >>   ##
> >>   { 'command': 'query-colo-status',
> >> -  'returns': 'COLOStatus' }
> >> +  'returns': 'COLOStatus',
> >> +  'if': 'CONFIG_REPLICATION' }
> >>
> >>   ##
> >>   # @migrate-recover:
> >> diff --git a/stubs/colo.c b/stubs/colo.c new file mode 100644 index
> >> 0000000000..5a02540baa
> >> --- /dev/null
> >> +++ b/stubs/colo.c
> >> @@ -0,0 +1,46 @@
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/notify.h"
> >> +#include "net/colo-compare.h"
> >> +#include "migration/colo.h"
> >> +#include "qapi/error.h"
> >> +#include "qapi/qapi-commands-migration.h"
> >> +
> >> +void colo_compare_cleanup(void)
> >> +{
> >> +    abort();
> >> +}
> >> +
> >> +void colo_shutdown(void)
> >> +{
> >> +    abort();
> >> +}
> >> +
> >> +void *colo_process_incoming_thread(void *opaque) {
> >> +    abort();
> >> +}
> >> +
> >> +void colo_checkpoint_notify(void *opaque) {
> >> +    abort();
> >> +}
> >> +
> >> +void migrate_start_colo_process(MigrationState *s) {
> >> +    abort();
> >> +}
> >> +
> >> +bool migration_in_colo_state(void)
> >> +{
> >> +    return false;
> >> +}
> >> +
> >> +bool migration_incoming_in_colo_state(void)
> >> +{
> >> +    return false;
> >> +}
> >> +
> >> +void qmp_x_colo_lost_heartbeat(Error **errp) {
> >> +    error_setg(errp, "COLO support is not built in"); }
> >> diff --git a/stubs/meson.build b/stubs/meson.build index
> >> b2b5956d97..8412cad15f 100644
> >> --- a/stubs/meson.build
> >> +++ b/stubs/meson.build
> >> @@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
> >>   stub_ss.add(files('target-monitor-defs.c'))
> >>   stub_ss.add(files('trace-control.c'))
> >>   stub_ss.add(files('uuid.c'))
> >> +stub_ss.add(files('colo.c'))
> >>   stub_ss.add(files('vmstate.c'))
> >>   stub_ss.add(files('vm-stop.c'))
> >>   stub_ss.add(files('win32-kbd-hook.c'))
> >> --
> >> 2.34.1
> >
> 
> --
> Best regards,
> Vladimir
> 

On 14.04.23 04:24, Zhang, Chen wrote:
>> So, if I want to have an option to disable all COLO modules, do you mean it
>> should be additional --disable-colo option? Or better keep one option --
>> disable-replication (and, maybe just rename to to --disable-colo)?
> I think keep the option --disable-replication is enough.
> Generally speaking, these three modules do not belong to COLO, It has been decoupled at the time of design.
> Instead, COLO is formed when these three modules are used in combination.

But it's not enough to me, I want to have a possibility to not build the subsystem I don't need.

-- 
Best regards,
Vladimir

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Friday, April 14, 2023 5:51 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; pbonzini@redhat.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; lizhijian@fujitsu.com;
> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
> plotnikov@yandex-team.ru
> Subject: Re: [PATCH] replication: compile out some staff when replication is
> not configured
> 
> On 14.04.23 04:24, Zhang, Chen wrote:
> >> So, if I want to have an option to disable all COLO modules, do you
> >> mean it should be additional --disable-colo option? Or better keep
> >> one option -- disable-replication (and, maybe just rename to to --disable-
> colo)?
> > I think keep the option --disable-replication is enough.
> > Generally speaking, these three modules do not belong to COLO, It has
> been decoupled at the time of design.
> > Instead, COLO is formed when these three modules are used in
> combination.
> 
> But it's not enough to me, I want to have a possibility to not build the
> subsystem I don't need.

As I said, COLO not a specific subsystem, It is a usage of three general subsystems.
Let's back to this patch, it try to not build block replication when not configured. It's OK.
Although COLO may be the only user of replication,  but can't assume all the COLO used subsystem
not needed, even have a --disable-colo. For example in this patch disabled the net/filter-mirror/redirector....
Qemu network filter is a general framework with many submodules: filter-buffer/replay/mirror/rewriter.....
Logically speaking, It is completely irrelevant with COLO.

Thanks
Chen


> 
> --
> Best regards,
> Vladimir

On 16.04.23 21:44, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Friday, April 14, 2023 5:51 PM
>> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
>> Cc: qemu-block@nongnu.org; pbonzini@redhat.com; armbru@redhat.com;
>> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
>> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
>> Hailiang <zhanghailiang@xfusion.com>; lizhijian@fujitsu.com;
>> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
>> plotnikov@yandex-team.ru
>> Subject: Re: [PATCH] replication: compile out some staff when replication is
>> not configured
>>
>> On 14.04.23 04:24, Zhang, Chen wrote:
>>>> So, if I want to have an option to disable all COLO modules, do you
>>>> mean it should be additional --disable-colo option? Or better keep
>>>> one option -- disable-replication (and, maybe just rename to to --disable-
>> colo)?
>>> I think keep the option --disable-replication is enough.
>>> Generally speaking, these three modules do not belong to COLO, It has
>> been decoupled at the time of design.
>>> Instead, COLO is formed when these three modules are used in
>> combination.
>>
>> But it's not enough to me, I want to have a possibility to not build the
>> subsystem I don't need.
> 
> As I said, COLO not a specific subsystem, It is a usage of three general subsystems.
> Let's back to this patch, it try to not build block replication when not configured. It's OK.
> Although COLO may be the only user of replication,  but can't assume all the COLO used subsystem
> not needed, even have a --disable-colo. For example in this patch disabled the net/filter-mirror/redirector....
> Qemu network filter is a general framework with many submodules: filter-buffer/replay/mirror/rewriter.....
> Logically speaking, It is completely irrelevant with COLO.
> 

OK, understand now, thanks for explanation!


-- 
Best regards,
Vladimir

On 17.04.23 20:19, Vladimir Sementsov-Ogievskiy wrote:
> On 16.04.23 21:44, Zhang, Chen wrote:
>>
>>
>>> -----Original Message-----
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> Sent: Friday, April 14, 2023 5:51 PM
>>> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
>>> Cc: qemu-block@nongnu.org; pbonzini@redhat.com; armbru@redhat.com;
>>> eblake@redhat.com; jasowang@redhat.com; dgilbert@redhat.com;
>>> quintela@redhat.com; hreitz@redhat.com; kwolf@redhat.com; Zhang,
>>> Hailiang <zhanghailiang@xfusion.com>; lizhijian@fujitsu.com;
>>> wencongyang2@huawei.com; xiechanglong.d@gmail.com; den-
>>> plotnikov@yandex-team.ru
>>> Subject: Re: [PATCH] replication: compile out some staff when replication is
>>> not configured
>>>
>>> On 14.04.23 04:24, Zhang, Chen wrote:
>>>>> So, if I want to have an option to disable all COLO modules, do you
>>>>> mean it should be additional --disable-colo option? Or better keep
>>>>> one option -- disable-replication (and, maybe just rename to to --disable-
>>> colo)?
>>>> I think keep the option --disable-replication is enough.
>>>> Generally speaking, these three modules do not belong to COLO, It has
>>> been decoupled at the time of design.
>>>> Instead, COLO is formed when these three modules are used in
>>> combination.
>>>
>>> But it's not enough to me, I want to have a possibility to not build the
>>> subsystem I don't need.
>>
>> As I said, COLO not a specific subsystem, It is a usage of three general subsystems.
>> Let's back to this patch, it try to not build block replication when not configured. It's OK.
>> Although COLO may be the only user of replication,  but can't assume all the COLO used subsystem
>> not needed, even have a --disable-colo. For example in this patch disabled the net/filter-mirror/redirector....
>> Qemu network filter is a general framework with many submodules: filter-buffer/replay/mirror/rewriter.....
>> Logically speaking, It is completely irrelevant with COLO.
>>
> 
> OK, understand now, thanks for explanation!
> 
> 

Hmm, OK, filters are separate.

But what's the sense in COLO subsystem when replication is disabled?

We have an explicit check in migration.c in migrate_caps_check():

#ifndef CONFIG_REPLICATION
     if (cap_list[MIGRATION_CAPABILITY_X_COLO]) {
         error_setg(errp, "QEMU compiled without replication module"
                    " can't enable COLO");
         error_append_hint(errp, "Please enable replication before COLO.\n");
         return false;
     }
#endif


So, x-colo capability - an entry point to COLO can't be used anyway. So, no reason to build it in?

-- 
Best regards,
Vladimir

Just a few minor nits

On 4/11/23 5:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> Don't compile-in replication-related files when replication is disabled
> in config.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> Hi all!
> 
> I'm unsure, should it be actually separate
> --disable-colo / --enable-colo options or it's really used only together
> with replication staff.. So, I decided to start with simpler variant.

You probably meant 'stuff' and not 'staff' in the commit message and 
here as well?

> 
>   block/meson.build     |  2 +-
>   migration/meson.build |  6 ++++--
>   net/meson.build       |  8 ++++----
>   qapi/migration.json   |  6 ++++--
>   stubs/colo.c          | 46 +++++++++++++++++++++++++++++++++++++++++++
>   stubs/meson.build     |  1 +
>   6 files changed, 60 insertions(+), 9 deletions(-)
>   create mode 100644 stubs/colo.c
> 
> diff --git a/block/meson.build b/block/meson.build
> index 382bec0e7d..b9a72e219b 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -84,7 +84,7 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c')
>   block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
>   block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
>   block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c'))
> -if not get_option('replication').disabled()
> +if get_option('replication').allowed()
>     block_ss.add(files('replication.c'))
>   endif
>   block_ss.add(when: libaio, if_true: files('linux-aio.c'))
> diff --git a/migration/meson.build b/migration/meson.build
> index 0d1bb9f96e..8180eaea7b 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -13,8 +13,6 @@ softmmu_ss.add(files(
>     'block-dirty-bitmap.c',
>     'channel.c',
>     'channel-block.c',
> -  'colo-failover.c',
> -  'colo.c',
>     'exec.c',
>     'fd.c',
>     'global_state.c',
> @@ -29,6 +27,10 @@ softmmu_ss.add(files(
>     'threadinfo.c',
>   ), gnutls)
>   
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo.c', 'colo-failover.c'))
> +endif
> +
>   softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
>   if get_option('live_block_migration').allowed()
>     softmmu_ss.add(files('block.c'))
> diff --git a/net/meson.build b/net/meson.build
> index 87afca3e93..634ab71cc6 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -1,13 +1,9 @@
>   softmmu_ss.add(files(
>     'announce.c',
>     'checksum.c',
> -  'colo-compare.c',
> -  'colo.c',
>     'dump.c',
>     'eth.c',
>     'filter-buffer.c',
> -  'filter-mirror.c',
> -  'filter-rewriter.c',
>     'filter.c',
>     'hub.c',
>     'net-hmp-cmds.c',
> @@ -19,6 +15,10 @@ softmmu_ss.add(files(
>     'util.c',
>   ))
>   
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo-compare.c', 'colo.c', 'filter-rewriter.c', 'filter-mirror.c'))
> +endif
> +
>   softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>   
>   if have_l2tpv3
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..5b81e09369 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1685,7 +1685,8 @@
>   ##
>   { 'struct': 'COLOStatus',
>     'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> -            'reason': 'COLOExitReason' } }
> +            'reason': 'COLOExitReason' },
> +  'if': 'CONFIG_REPLICATION' }
>   
>   ##
>   # @query-colo-status:
> @@ -1702,7 +1703,8 @@
>   # Since: 3.1
>   ##
>   { 'command': 'query-colo-status',
> -  'returns': 'COLOStatus' }
> +  'returns': 'COLOStatus',
> +  'if': 'CONFIG_REPLICATION' }
>   
>   ##
>   # @migrate-recover:
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..5a02540baa
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,46 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_compare_cleanup(void)
> +{
> +    abort();
> +}
> +
> +void colo_shutdown(void)
> +{
> +    abort();
> +}
> +
> +void *colo_process_incoming_thread(void *opaque)
> +{
> +    abort();
> +}
> +
> +void colo_checkpoint_notify(void *opaque)
> +{
> +    abort();
> +}
> +
> +void migrate_start_colo_process(MigrationState *s)
> +{
> +    abort();
> +}
> +
> +bool migration_in_colo_state(void)
> +{
> +    return false;
> +}
> +
> +bool migration_incoming_in_colo_state(void)
> +{
> +    return false;
> +}
> +
> +void qmp_x_colo_lost_heartbeat(Error **errp)
> +{
> +    error_setg(errp, "COLO support is not built in");

Maybe 'built-in' with a dash for consistency with usb-dev-stub?

> +}
> diff --git a/stubs/meson.build b/stubs/meson.build
> index b2b5956d97..8412cad15f 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -45,6 +45,7 @@ stub_ss.add(files('target-get-monitor-def.c'))
>   stub_ss.add(files('target-monitor-defs.c'))
>   stub_ss.add(files('trace-control.c'))
>   stub_ss.add(files('uuid.c'))
> +stub_ss.add(files('colo.c'))
>   stub_ss.add(files('vmstate.c'))
>   stub_ss.add(files('vm-stop.c'))
>   stub_ss.add(files('win32-kbd-hook.c'))