[PATCH v2 0/4] COLO: improve build options

[PATCH v2 0/4] COLO: improve build options

[Vladimir Sementsov-Ogievskiy]

Hi all!

COLO substem seems to be useless when CONFIG_REPLICATION is unset, as we
simply don't allow to set x-colo capability in this case. So, let's not
compile in unreachable code and interface we cannot use when
CONFIG_REPLICATION is unset.

Also, provide personal configure option for COLO Proxy subsystem.

v1 was 
[PATCH] replication: compile out some staff when replication is not configured
Supersedes: <20230411145112.497785-1-vsementsov@yandex-team.ru>

Vladimir Sementsov-Ogievskiy (4):
  block/meson.build: prefer positive condition for replication
  scripts/qapi: allow optional experimental enum values
  build: move COLO under CONFIG_REPLICATION
  configure: add --disable-colo-filters option

 block/meson.build              |  2 +-
 hmp-commands.hx                |  2 ++
 meson.build                    |  1 +
 meson_options.txt              |  2 ++
 migration/colo.c               |  6 +++++
 migration/meson.build          |  6 +++--
 migration/migration-hmp-cmds.c |  2 ++
 migration/migration.c          | 19 +++-----------
 net/meson.build                | 16 +++++++++---
 qapi/migration.json            | 12 ++++++---
 scripts/meson-buildoptions.sh  |  3 +++
 scripts/qapi/types.py          |  2 ++
 stubs/colo.c                   | 47 ++++++++++++++++++++++++++++++++++
 stubs/meson.build              |  1 +
 14 files changed, 95 insertions(+), 26 deletions(-)
 create mode 100644 stubs/colo.c

-- 
2.34.1

[PATCH v2 1/4] block/meson.build: prefer positive condition for replication

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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'))
-- 
2.34.1

Re: [PATCH v2 1/4] block/meson.build: prefer positive condition for replication

[Juan Quintela]

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


Reviewed-by: Juan Quintela <quintela@redhat.com>

> ---
>  block/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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'))

Re: [PATCH v2 1/4] block/meson.build: prefer positive condition for replication

[Philippe Mathieu-Daudé]

On 20/4/23 00:52, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   block/meson.build | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 2/4] scripts/qapi: allow optional experimental enum values

[Vladimir Sementsov-Ogievskiy]

To be used in the next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 scripts/qapi/types.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index c39d054d2c..18f8734047 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -61,10 +61,12 @@ def gen_enum_lookup(name: str,
 
         special_features = gen_special_features(memb.features)
         if special_features != '0':
+            feats += memb.ifcond.gen_if()
             feats += mcgen('''
         [%(index)s] = %(special_features)s,
 ''',
                            index=index, special_features=special_features)
+            feats += memb.ifcond.gen_endif()
 
     if feats:
         ret += mcgen('''
-- 
2.34.1

Re: [PATCH v2 2/4] scripts/qapi: allow optional experimental enum values

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> To be used in the next commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I am cc'd, but this is black magic to me.

Later, Juan.

Re: [PATCH v2 2/4] scripts/qapi: allow optional experimental enum values

[Eric Blake]

On Thu, Apr 20, 2023 at 01:52:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> To be used in the next commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  scripts/qapi/types.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index c39d054d2c..18f8734047 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -61,10 +61,12 @@ def gen_enum_lookup(name: str,
>  
>          special_features = gen_special_features(memb.features)
>          if special_features != '0':
> +            feats += memb.ifcond.gen_if()
>              feats += mcgen('''
>          [%(index)s] = %(special_features)s,
>  ''',
>                             index=index, special_features=special_features)
> +            feats += memb.ifcond.gen_endif()

Perhaps Markus will have a comment here; but in general, changes to
the QAPI that don't have accompanying changes in the testsuite are
hard to prove that they do something useful.

At a minimum, the commit message should at least say what sort of
things are not permitted without this patch that are now possible,
rather than making me figure out what the next patch uses that failed
the QAPI generator without this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/4] scripts/qapi: allow optional experimental enum values

[Vladimir Sementsov-Ogievskiy]

On 20.04.23 17:43, Eric Blake wrote:
> On Thu, Apr 20, 2023 at 01:52:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> To be used in the next commit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   scripts/qapi/types.py | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> index c39d054d2c..18f8734047 100644
>> --- a/scripts/qapi/types.py
>> +++ b/scripts/qapi/types.py
>> @@ -61,10 +61,12 @@ def gen_enum_lookup(name: str,
>>   
>>           special_features = gen_special_features(memb.features)
>>           if special_features != '0':
>> +            feats += memb.ifcond.gen_if()
>>               feats += mcgen('''
>>           [%(index)s] = %(special_features)s,
>>   ''',
>>                              index=index, special_features=special_features)
>> +            feats += memb.ifcond.gen_endif()
> 
> Perhaps Markus will have a comment here; but in general, changes to
> the QAPI that don't have accompanying changes in the testsuite are
> hard to prove that they do something useful.
> 
> At a minimum, the commit message should at least say what sort of
> things are not permitted without this patch that are now possible,
> rather than making me figure out what the next patch uses that failed
> the QAPI generator without this patch.
> 

OK, I'll improve the commit message if we decide to keep patch 3 as is.

Actually patch would be more obvious if have a bit more code context: just above this same gen_if()s are called to generate #ifdefs for enum members.


-- 
Best regards,
Vladimir

[PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Vladimir Sementsov-Ogievskiy]

We don't allow to use x-colo capability when replication is not
configured. So, no reason to build COLO when replication is disabled,
it's unusable in this case.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hmp-commands.hx                |  2 ++
 migration/colo.c               |  6 +++++
 migration/meson.build          |  6 +++--
 migration/migration-hmp-cmds.c |  2 ++
 migration/migration.c          | 19 +++-----------
 net/meson.build                |  5 +++-
 qapi/migration.json            | 12 ++++++---
 stubs/colo.c                   | 47 ++++++++++++++++++++++++++++++++++
 stubs/meson.build              |  1 +
 9 files changed, 78 insertions(+), 22 deletions(-)
 create mode 100644 stubs/colo.c

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb85ee1d26..fbd0932232 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1035,6 +1035,7 @@ SRST
   migration (or once already in postcopy).
 ERST
 
+#ifdef CONFIG_REPLICATION
     {
         .name       = "x_colo_lost_heartbeat",
         .args_type  = "",
@@ -1043,6 +1044,7 @@ ERST
                       "a failover or takeover is needed.",
         .cmd = hmp_x_colo_lost_heartbeat,
     },
+#endif
 
 SRST
 ``x_colo_lost_heartbeat``
diff --git a/migration/colo.c b/migration/colo.c
index 0716e64689..089c491d70 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -196,6 +196,12 @@ COLOMode get_colo_mode(void)
     }
 }
 
+bool migrate_colo_enabled(void)
+{
+    MigrationState *s = migrate_get_current();
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
+}
+
 void colo_do_failover(void)
 {
     /* Make sure VM stopped while failover happened. */
diff --git a/migration/meson.build b/migration/meson.build
index 0d1bb9f96e..3fccf79f12 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-failover.c', 'colo.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/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 72519ea99f..4601c48f41 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -640,6 +640,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+#ifdef CONFIG_REPLICATION
 void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
@@ -647,6 +648,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
     qmp_x_colo_lost_heartbeat(&err);
     hmp_handle_error(mon, err);
 }
+#endif
 
 typedef struct HMPMigrationStatus {
     QEMUTimer *timer;
diff --git a/migration/migration.c b/migration/migration.c
index bda4789193..2382958364 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -165,7 +165,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
     MIGRATION_CAPABILITY_RDMA_PIN_ALL,
     MIGRATION_CAPABILITY_COMPRESS,
     MIGRATION_CAPABILITY_XBZRLE,
+#ifdef CONFIG_REPLICATION
     MIGRATION_CAPABILITY_X_COLO,
+#endif
     MIGRATION_CAPABILITY_VALIDATE_UUID,
     MIGRATION_CAPABILITY_ZERO_COPY_SEND);
 
@@ -1329,15 +1331,6 @@ static bool migrate_caps_check(bool *cap_list,
     }
 #endif
 
-#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
-
     if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
@@ -3577,12 +3570,6 @@ fail:
                       MIGRATION_STATUS_FAILED);
 }
 
-bool migrate_colo_enabled(void)
-{
-    MigrationState *s = migrate_get_current();
-    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
-}
-
 typedef enum MigThrError {
     /* No error detected */
     MIG_THR_ERR_NONE = 0,
@@ -4537,7 +4524,9 @@ static Property migration_properties[] = {
     DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
     DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
                         MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
+#ifdef CONFIG_REPLICATION
     DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
+#endif
     DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
     DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
     DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
diff --git a/net/meson.build b/net/meson.build
index 87afca3e93..38d50b8c96 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -1,7 +1,6 @@
 softmmu_ss.add(files(
   'announce.c',
   'checksum.c',
-  'colo-compare.c',
   'colo.c',
   'dump.c',
   'eth.c',
@@ -19,6 +18,10 @@ softmmu_ss.add(files(
   'util.c',
 ))
 
+if get_option('replication').allowed()
+  softmmu_ss.add(files('colo-compare.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..93863fa88c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -486,7 +486,8 @@
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
            'compress', 'events', 'postcopy-ram',
-           { 'name': 'x-colo', 'features': [ 'unstable' ] },
+           { 'name': 'x-colo', 'features': [ 'unstable' ],
+             'if': 'CONFIG_REPLICATION' },
            'release-ram',
            'block', 'return-path', 'pause-before-switchover', 'multifd',
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
@@ -1409,7 +1410,8 @@
 #
 ##
 { 'command': 'x-colo-lost-heartbeat',
-  'features': [ 'unstable' ] }
+  'features': [ 'unstable' ],
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @migrate_cancel:
@@ -1685,7 +1687,8 @@
 ##
 { 'struct': 'COLOStatus',
   'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
-            'reason': 'COLOExitReason' } }
+            'reason': 'COLOExitReason' },
+  'if': 'CONFIG_REPLICATION' }
 
 ##
 # @query-colo-status:
@@ -1702,7 +1705,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..45c8ac0cc6
--- /dev/null
+++ b/stubs/colo.c
@@ -0,0 +1,47 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+#include "migration/colo.h"
+#include "migration/migration.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;
+}
+
+bool migrate_colo_enabled(void)
+{
+    return false;
+}
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

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> +bool migrate_colo_enabled(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> +}

I consolidated all the capabilities check on my series on the list on
options.c, so this will go there.

> diff --git a/migration/migration.c b/migration/migration.c
> index bda4789193..2382958364 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -165,7 +165,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_RDMA_PIN_ALL,
>      MIGRATION_CAPABILITY_COMPRESS,
>      MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
>      MIGRATION_CAPABILITY_X_COLO,
> +#endif

Why?

I very much preffer the capability to be there and just fail when we try
to enable it.  That way we only need the #ifdef in one place and not all
over the place.

>      MIGRATION_CAPABILITY_VALIDATE_UUID,
>      MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>  
> @@ -1329,15 +1331,6 @@ static bool migrate_caps_check(bool *cap_list,
>      }
>  #endif
>  
> -#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
> -

See, like this O:-)

> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..93863fa88c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -486,7 +486,8 @@
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>             'compress', 'events', 'postcopy-ram',
> -           { 'name': 'x-colo', 'features': [ 'unstable' ] },
> +           { 'name': 'x-colo', 'features': [ 'unstable' ],
> +             'if': 'CONFIG_REPLICATION' },
>             'release-ram',
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',


Aha, It is for this that you changed the black magic on the previous
patch. Looks ok from my ignorance.  As said before, I would not remove
the capability, left it the way it was.

You have less code (less #ifdefs that you just had to add), and
enabling/disabling checking capabilities don't need anything from replication.

>  #
>  ##
>  { 'command': 'x-colo-lost-heartbeat',
> -  'features': [ 'unstable' ] }
> +  'features': [ 'unstable' ],
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @migrate_cancel:
> @@ -1685,7 +1687,8 @@
>  ##
>  { 'struct': 'COLOStatus',
>    'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> -            'reason': 'COLOExitReason' } }
> +            'reason': 'COLOExitReason' },
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @query-colo-status:
> @@ -1702,7 +1705,8 @@
>  # Since: 3.1
>  ##
>  { 'command': 'query-colo-status',
> -  'returns': 'COLOStatus' }
> +  'returns': 'COLOStatus',
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @migrate-recover:

Disabling the command is ok for me.

> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 0000000000..45c8ac0cc6

...

> +bool migrate_colo_enabled(void)
> +{
> +    return false;
> +}

You don't need this one if you follow my idea.


Notice that I fully agree with being able to disable colo O:-)

Later, Juan.

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Vladimir Sementsov-Ogievskiy]

On 20.04.23 13:03, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> We don't allow to use x-colo capability when replication is not
>> configured. So, no reason to build COLO when replication is disabled,
>> it's unusable in this case.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
>> +bool migrate_colo_enabled(void)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
>> +}
> 
> I consolidated all the capabilities check on my series on the list on
> options.c, so this will go there.
> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index bda4789193..2382958364 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -165,7 +165,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>>       MIGRATION_CAPABILITY_RDMA_PIN_ALL,
>>       MIGRATION_CAPABILITY_COMPRESS,
>>       MIGRATION_CAPABILITY_XBZRLE,
>> +#ifdef CONFIG_REPLICATION
>>       MIGRATION_CAPABILITY_X_COLO,
>> +#endif
> 
> Why?
> 
> I very much preffer the capability to be there and just fail when we try
> to enable it.  That way we only need the #ifdef in one place and not all
> over the place.
> 
>>       MIGRATION_CAPABILITY_VALIDATE_UUID,
>>       MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>>   
>> @@ -1329,15 +1331,6 @@ static bool migrate_caps_check(bool *cap_list,
>>       }
>>   #endif
>>   
>> -#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
>> -
> 
> See, like this O:-)
> 
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c84fa10e86..93863fa88c 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -486,7 +486,8 @@
>>   { 'enum': 'MigrationCapability',
>>     'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>              'compress', 'events', 'postcopy-ram',
>> -           { 'name': 'x-colo', 'features': [ 'unstable' ] },
>> +           { 'name': 'x-colo', 'features': [ 'unstable' ],
>> +             'if': 'CONFIG_REPLICATION' },
>>              'release-ram',
>>              'block', 'return-path', 'pause-before-switchover', 'multifd',
>>              'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> 
> 
> Aha, It is for this that you changed the black magic on the previous
> patch. Looks ok from my ignorance.  As said before, I would not remove
> the capability, left it the way it was.
> 
> You have less code (less #ifdefs that you just had to add), and
> enabling/disabling checking capabilities don't need anything from replication.

Yes, I had a sense of doubt while adding these #ifdefs.

Still, on the other hand I feel that it's strange to have public interface which only can say "I'm not built in" :)

Actually with my way, we have just two #ifdefs instead of one. Seems, not too many. And instead of "I'm not supported" error we just not include the public interface for unsupported feature. It seems to be better user experience. What do you think?


-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 20.04.23 13:03, Juan Quintela wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>>> We don't allow to use x-colo capability when replication is not
>>> configured. So, no reason to build COLO when replication is disabled,
>>> it's unusable in this case.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> 
>>> +bool migrate_colo_enabled(void)
>>> +{
>>> +    MigrationState *s = migrate_get_current();
>>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
>>> +}

>> Aha, It is for this that you changed the black magic on the previous
>> patch. Looks ok from my ignorance.  As said before, I would not remove
>> the capability, left it the way it was.
>> You have less code (less #ifdefs that you just had to add), and
>> enabling/disabling checking capabilities don't need anything from replication.
>
> Yes, I had a sense of doubt while adding these #ifdefs.
>
> Still, on the other hand I feel that it's strange to have public interface which only can say "I'm not built in" :)
>
> Actually with my way, we have just two #ifdefs instead of one. Seems,
> not too many. And instead of "I'm not supported" error we just not
> include the public interface for unsupported feature. It seems to be
> better user experience. What do you think?

I preffer the regularity that all capabilities are the same, and the
only place to look if something is disabled is a single place.

But on the other hand, the main user is libvirt, so

Daniel, what does libvirt preffers?

/me guess that they have to do both anyways to detect old versions, but
who knows.

Later, Juan.

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@yandex-team.ru) wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hmp-commands.hx                |  2 ++
>  migration/colo.c               |  6 +++++
>  migration/meson.build          |  6 +++--
>  migration/migration-hmp-cmds.c |  2 ++
>  migration/migration.c          | 19 +++-----------
>  net/meson.build                |  5 +++-
>  qapi/migration.json            | 12 ++++++---
>  stubs/colo.c                   | 47 ++++++++++++++++++++++++++++++++++
>  stubs/meson.build              |  1 +
>  9 files changed, 78 insertions(+), 22 deletions(-)
>  create mode 100644 stubs/colo.c
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb85ee1d26..fbd0932232 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1035,6 +1035,7 @@ SRST
>    migration (or once already in postcopy).
>  ERST
>  
> +#ifdef CONFIG_REPLICATION
>      {
>          .name       = "x_colo_lost_heartbeat",
>          .args_type  = "",
> @@ -1043,6 +1044,7 @@ ERST
>                        "a failover or takeover is needed.",
>          .cmd = hmp_x_colo_lost_heartbeat,
>      },
> +#endif

We seem to be inconsistent about whether the ifdef includes the
SRST/ERST doc section; some ifdef do and some don't; and thus
it depends whether or not you want the command documented
even though it's compiled out.

I think it's probably OK, but maybe worth reconsidering:

Acked-by: Dr. David Alan Gilbert <dave@treblig.org>

>  SRST
>  ``x_colo_lost_heartbeat``
> diff --git a/migration/colo.c b/migration/colo.c
> index 0716e64689..089c491d70 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -196,6 +196,12 @@ COLOMode get_colo_mode(void)
>      }
>  }
>  
> +bool migrate_colo_enabled(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> +}
> +
>  void colo_do_failover(void)
>  {
>      /* Make sure VM stopped while failover happened. */
> diff --git a/migration/meson.build b/migration/meson.build
> index 0d1bb9f96e..3fccf79f12 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-failover.c', 'colo.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/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 72519ea99f..4601c48f41 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -640,6 +640,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, err);
>  }
>  
> +#ifdef CONFIG_REPLICATION
>  void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> @@ -647,6 +648,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
>      qmp_x_colo_lost_heartbeat(&err);
>      hmp_handle_error(mon, err);
>  }
> +#endif
>  
>  typedef struct HMPMigrationStatus {
>      QEMUTimer *timer;
> diff --git a/migration/migration.c b/migration/migration.c
> index bda4789193..2382958364 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -165,7 +165,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_RDMA_PIN_ALL,
>      MIGRATION_CAPABILITY_COMPRESS,
>      MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
>      MIGRATION_CAPABILITY_X_COLO,
> +#endif
>      MIGRATION_CAPABILITY_VALIDATE_UUID,
>      MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>  
> @@ -1329,15 +1331,6 @@ static bool migrate_caps_check(bool *cap_list,
>      }
>  #endif
>  
> -#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
> -
>      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>          /* This check is reasonably expensive, so only when it's being
>           * set the first time, also it's only the destination that needs
> @@ -3577,12 +3570,6 @@ fail:
>                        MIGRATION_STATUS_FAILED);
>  }
>  
> -bool migrate_colo_enabled(void)
> -{
> -    MigrationState *s = migrate_get_current();
> -    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> -}
> -
>  typedef enum MigThrError {
>      /* No error detected */
>      MIG_THR_ERR_NONE = 0,
> @@ -4537,7 +4524,9 @@ static Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
>      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
>                          MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
>      DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
>      DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
>      DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
>      DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> diff --git a/net/meson.build b/net/meson.build
> index 87afca3e93..38d50b8c96 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -1,7 +1,6 @@
>  softmmu_ss.add(files(
>    'announce.c',
>    'checksum.c',
> -  'colo-compare.c',
>    'colo.c',
>    'dump.c',
>    'eth.c',
> @@ -19,6 +18,10 @@ softmmu_ss.add(files(
>    'util.c',
>  ))
>  
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo-compare.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..93863fa88c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -486,7 +486,8 @@
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>             'compress', 'events', 'postcopy-ram',
> -           { 'name': 'x-colo', 'features': [ 'unstable' ] },
> +           { 'name': 'x-colo', 'features': [ 'unstable' ],
> +             'if': 'CONFIG_REPLICATION' },
>             'release-ram',
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> @@ -1409,7 +1410,8 @@
>  #
>  ##
>  { 'command': 'x-colo-lost-heartbeat',
> -  'features': [ 'unstable' ] }
> +  'features': [ 'unstable' ],
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @migrate_cancel:
> @@ -1685,7 +1687,8 @@
>  ##
>  { 'struct': 'COLOStatus',
>    'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> -            'reason': 'COLOExitReason' } }
> +            'reason': 'COLOExitReason' },
> +  'if': 'CONFIG_REPLICATION' }
>  
>  ##
>  # @query-colo-status:
> @@ -1702,7 +1705,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..45c8ac0cc6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,47 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.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;
> +}
> +
> +bool migrate_colo_enabled(void)
> +{
> +    return false;
> +}
> 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
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

RE: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Thursday, April 20, 2023 6:53 AM
> To: qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy <vsementsov@yandex-
> team.ru>
> Subject: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
> 
> We don't allow to use x-colo capability when replication is not configured. So,
> no reason to build COLO when replication is disabled, it's unusable in this
> case.

Yes, you are right for current status. Because COLO best practices is replication + colo live migration + colo proxy.
But doesn't mean it has to be done in all scenarios as I explanation in V1.
The better way is allow to use x-colo capability firstly, and separate this patch
with two config options: --disable-replication  and --disable-x-colo.

Thanks
Chen

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hmp-commands.hx                |  2 ++
>  migration/colo.c               |  6 +++++
>  migration/meson.build          |  6 +++--
>  migration/migration-hmp-cmds.c |  2 ++
>  migration/migration.c          | 19 +++-----------
>  net/meson.build                |  5 +++-
>  qapi/migration.json            | 12 ++++++---
>  stubs/colo.c                   | 47 ++++++++++++++++++++++++++++++++++
>  stubs/meson.build              |  1 +
>  9 files changed, 78 insertions(+), 22 deletions(-)  create mode 100644
> stubs/colo.c
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx index
> bb85ee1d26..fbd0932232 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1035,6 +1035,7 @@ SRST
>    migration (or once already in postcopy).
>  ERST
> 
> +#ifdef CONFIG_REPLICATION
>      {
>          .name       = "x_colo_lost_heartbeat",
>          .args_type  = "",
> @@ -1043,6 +1044,7 @@ ERST
>                        "a failover or takeover is needed.",
>          .cmd = hmp_x_colo_lost_heartbeat,
>      },
> +#endif
> 
>  SRST
>  ``x_colo_lost_heartbeat``
> diff --git a/migration/colo.c b/migration/colo.c index
> 0716e64689..089c491d70 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -196,6 +196,12 @@ COLOMode get_colo_mode(void)
>      }
>  }
> 
> +bool migrate_colo_enabled(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> +}
> +
>  void colo_do_failover(void)
>  {
>      /* Make sure VM stopped while failover happened. */ diff --git
> a/migration/meson.build b/migration/meson.build index
> 0d1bb9f96e..3fccf79f12 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-failover.c', 'colo.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/migration/migration-hmp-cmds.c b/migration/migration-hmp-
> cmds.c index 72519ea99f..4601c48f41 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -640,6 +640,7 @@ void hmp_migrate_start_postcopy(Monitor *mon,
> const QDict *qdict)
>      hmp_handle_error(mon, err);
>  }
> 
> +#ifdef CONFIG_REPLICATION
>  void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)  {
>      Error *err = NULL;
> @@ -647,6 +648,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon,
> const QDict *qdict)
>      qmp_x_colo_lost_heartbeat(&err);
>      hmp_handle_error(mon, err);
>  }
> +#endif
> 
>  typedef struct HMPMigrationStatus {
>      QEMUTimer *timer;
> diff --git a/migration/migration.c b/migration/migration.c index
> bda4789193..2382958364 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -165,7 +165,9 @@
> INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_RDMA_PIN_ALL,
>      MIGRATION_CAPABILITY_COMPRESS,
>      MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
>      MIGRATION_CAPABILITY_X_COLO,
> +#endif
>      MIGRATION_CAPABILITY_VALIDATE_UUID,
>      MIGRATION_CAPABILITY_ZERO_COPY_SEND);
> 
> @@ -1329,15 +1331,6 @@ static bool migrate_caps_check(bool *cap_list,
>      }
>  #endif
> 
> -#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
> -
>      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>          /* This check is reasonably expensive, so only when it's being
>           * set the first time, also it's only the destination that needs @@ -
> 3577,12 +3570,6 @@ fail:
>                        MIGRATION_STATUS_FAILED);  }
> 
> -bool migrate_colo_enabled(void)
> -{
> -    MigrationState *s = migrate_get_current();
> -    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> -}
> -
>  typedef enum MigThrError {
>      /* No error detected */
>      MIG_THR_ERR_NONE = 0,
> @@ -4537,7 +4524,9 @@ static Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-postcopy-ram",
> MIGRATION_CAPABILITY_POSTCOPY_RAM),
>      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
>                          MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
>      DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
>      DEFINE_PROP_MIG_CAP("x-release-ram",
> MIGRATION_CAPABILITY_RELEASE_RAM),
>      DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
>      DEFINE_PROP_MIG_CAP("x-return-path",
> MIGRATION_CAPABILITY_RETURN_PATH),
> diff --git a/net/meson.build b/net/meson.build index
> 87afca3e93..38d50b8c96 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -1,7 +1,6 @@
>  softmmu_ss.add(files(
>    'announce.c',
>    'checksum.c',
> -  'colo-compare.c',
>    'colo.c',
>    'dump.c',
>    'eth.c',
> @@ -19,6 +18,10 @@ softmmu_ss.add(files(
>    'util.c',
>  ))
> 
> +if get_option('replication').allowed()
> +  softmmu_ss.add(files('colo-compare.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..93863fa88c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -486,7 +486,8 @@
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>             'compress', 'events', 'postcopy-ram',
> -           { 'name': 'x-colo', 'features': [ 'unstable' ] },
> +           { 'name': 'x-colo', 'features': [ 'unstable' ],
> +             'if': 'CONFIG_REPLICATION' },
>             'release-ram',
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', @@ -1409,7
> +1410,8 @@  #  ##  { 'command': 'x-colo-lost-heartbeat',
> -  'features': [ 'unstable' ] }
> +  'features': [ 'unstable' ],
> +  'if': 'CONFIG_REPLICATION' }
> 
>  ##
>  # @migrate_cancel:
> @@ -1685,7 +1687,8 @@
>  ##
>  { 'struct': 'COLOStatus',
>    'data': { 'mode': 'COLOMode', 'last-mode': 'COLOMode',
> -            'reason': 'COLOExitReason' } }
> +            'reason': 'COLOExitReason' },
> +  'if': 'CONFIG_REPLICATION' }
> 
>  ##
>  # @query-colo-status:
> @@ -1702,7 +1705,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..45c8ac0cc6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,47 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.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;
> +}
> +
> +bool migrate_colo_enabled(void)
> +{
> +    return false;
> +}
> 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

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Vladimir Sementsov-Ogievskiy]

On 21.04.23 06:02, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Thursday, April 20, 2023 6:53 AM
>> To: qemu-devel@nongnu.org
>> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
>> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
>> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
>> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
>> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
>> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
>> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy <vsementsov@yandex-
>> team.ru>
>> Subject: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
>>
>> We don't allow to use x-colo capability when replication is not configured. So,
>> no reason to build COLO when replication is disabled, it's unusable in this
>> case.
> 
> Yes, you are right for current status. Because COLO best practices is replication + colo live migration + colo proxy.
> But doesn't mean it has to be done in all scenarios as I explanation in V1.
> The better way is allow to use x-colo capability firstly, and separate this patch
> with two config options: --disable-replication  and --disable-x-colo.
> 

But what for? We for sure don't have such scenarios now (COLO without replication), as it's not allowed by far 7e934f5b27eee1b0d7 (by you and David).

If you think we need such scenario, I think it should be a separate series which reverts 7e934f5b27eee1b0d7 and adds corresponding test and probably documentation.


-- 
Best regards,
Vladimir

RE: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Friday, April 21, 2023 4:36 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> kwolf@redhat.com; lizhijian@fujitsu.com
> Subject: Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
> 
> On 21.04.23 06:02, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> Sent: Thursday, April 20, 2023 6:53 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: qemu-block@nongnu.org; michael.roth@amd.com;
> armbru@redhat.com;
> >> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com;
> Zhang,
> >> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> >> thuth@redhat.com; berrange@redhat.com;
> marcandre.lureau@redhat.com;
> >> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> >> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
> >> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex- team.ru>
> >> Subject: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
> >>
> >> We don't allow to use x-colo capability when replication is not
> >> configured. So, no reason to build COLO when replication is disabled,
> >> it's unusable in this case.
> >
> > Yes, you are right for current status. Because COLO best practices is
> replication + colo live migration + colo proxy.
> > But doesn't mean it has to be done in all scenarios as I explanation in V1.
> > The better way is allow to use x-colo capability firstly, and separate
> > this patch with two config options: --disable-replication  and --disable-x-
> colo.
> >
> 
> But what for? We for sure don't have such scenarios now (COLO without
> replication), as it's not allowed by far 7e934f5b27eee1b0d7 (by you and
> David).
> 
> If you think we need such scenario, I think it should be a separate series
> which reverts 7e934f5b27eee1b0d7 and adds corresponding test and
> probably documentation.

In the patch 7e934f5b27eee1b0d7 said it's for current independent disk mode,
And what we talked about before is the shared disk mode.
Rethink about the COLO shared disk mode, this feature still needs some enabling works.
It looks OK for now and separate the build options when enabling COLO shared disk mode.

Thanks
Chen

> 
> 
> --
> Best regards,
> Vladimir

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Vladimir Sementsov-Ogievskiy]

On 23.04.23 04:54, Zhang, Chen wrote:
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>> Sent: Friday, April 21, 2023 4:36 PM
>> To: Zhang, Chen<chen.zhang@intel.com>;qemu-devel@nongnu.org
>> Cc:qemu-block@nongnu.org;michael.roth@amd.com;armbru@redhat.com;
>> eblake@redhat.com;jasowang@redhat.com;quintela@redhat.com; Zhang,
>> Hailiang<zhanghailiang@xfusion.com>;philmd@linaro.org;
>> thuth@redhat.com;berrange@redhat.com;marcandre.lureau@redhat.com;
>> pbonzini@redhat.com;dave@treblig.org;hreitz@redhat.com;
>> kwolf@redhat.com;lizhijian@fujitsu.com
>> Subject: Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
>>
>> On 21.04.23 06:02, Zhang, Chen wrote:
>>>
>>>> -----Original Message-----
>>>> From: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>>>> Sent: Thursday, April 20, 2023 6:53 AM
>>>> To:qemu-devel@nongnu.org
>>>> Cc:qemu-block@nongnu.org;michael.roth@amd.com;
>> armbru@redhat.com;
>>>> eblake@redhat.com;jasowang@redhat.com;quintela@redhat.com;
>> Zhang,
>>>> Hailiang<zhanghailiang@xfusion.com>;philmd@linaro.org;
>>>> thuth@redhat.com;berrange@redhat.com;
>> marcandre.lureau@redhat.com;
>>>> pbonzini@redhat.com;dave@treblig.org;hreitz@redhat.com;
>>>> kwolf@redhat.com; Zhang, Chen<chen.zhang@intel.com>;
>>>> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy
>>>> <vsementsov@yandex- team.ru>
>>>> Subject: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
>>>>
>>>> We don't allow to use x-colo capability when replication is not
>>>> configured. So, no reason to build COLO when replication is disabled,
>>>> it's unusable in this case.
>>> Yes, you are right for current status. Because COLO best practices is
>> replication + colo live migration + colo proxy.
>>> But doesn't mean it has to be done in all scenarios as I explanation in V1.
>>> The better way is allow to use x-colo capability firstly, and separate
>>> this patch with two config options: --disable-replication  and --disable-x-
>> colo.
>> But what for? We for sure don't have such scenarios now (COLO without
>> replication), as it's not allowed by far 7e934f5b27eee1b0d7 (by you and
>> David).
>>
>> If you think we need such scenario, I think it should be a separate series
>> which reverts 7e934f5b27eee1b0d7 and adds corresponding test and
>> probably documentation.
> In the patch 7e934f5b27eee1b0d7 said it's for current independent disk mode,
> And what we talked about before is the shared disk mode.
> Rethink about the COLO shared disk mode, this feature still needs some enabling works.
> It looks OK for now and separate the build options when enabling COLO shared disk mode.

I've started working on this, and now I see, that check in the migrate_caps_check() is not the only place.

migration/colo.c has also several abort() points. For example, colo_process_checkpoint will simply abort if CONFIG_REPLICATION not defined.

So for sure, current code is not prepared to use COLO with REPLICATION disabled.

If this possibility is needed it requires more work. Personally, I don't think that possibility to enable COLO with disabled REPLICATION is really needed and I know nobody who need it, so that seems to be extra work.


-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 23.04.23 04:54, Zhang, Chen wrote:
>> 
>>> -----Original Message-----
>>> From: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>>> Sent: Friday, April 21, 2023 4:36 PM
>>> To: Zhang, Chen<chen.zhang@intel.com>;qemu-devel@nongnu.org
>>> Cc:qemu-block@nongnu.org;michael.roth@amd.com;armbru@redhat.com;
>>> eblake@redhat.com;jasowang@redhat.com;quintela@redhat.com; Zhang,
>>> Hailiang<zhanghailiang@xfusion.com>;philmd@linaro.org;
>>> thuth@redhat.com;berrange@redhat.com;marcandre.lureau@redhat.com;
>>> pbonzini@redhat.com;dave@treblig.org;hreitz@redhat.com;
>>> kwolf@redhat.com;lizhijian@fujitsu.com
>>> Subject: Re: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
>>>
>>> On 21.04.23 06:02, Zhang, Chen wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>>>>> Sent: Thursday, April 20, 2023 6:53 AM
>>>>> To:qemu-devel@nongnu.org
>>>>> Cc:qemu-block@nongnu.org;michael.roth@amd.com;
>>> armbru@redhat.com;
>>>>> eblake@redhat.com;jasowang@redhat.com;quintela@redhat.com;
>>> Zhang,
>>>>> Hailiang<zhanghailiang@xfusion.com>;philmd@linaro.org;
>>>>> thuth@redhat.com;berrange@redhat.com;
>>> marcandre.lureau@redhat.com;
>>>>> pbonzini@redhat.com;dave@treblig.org;hreitz@redhat.com;
>>>>> kwolf@redhat.com; Zhang, Chen<chen.zhang@intel.com>;
>>>>> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy
>>>>> <vsementsov@yandex- team.ru>
>>>>> Subject: [PATCH v2 3/4] build: move COLO under CONFIG_REPLICATION
>>>>>
>>>>> We don't allow to use x-colo capability when replication is not
>>>>> configured. So, no reason to build COLO when replication is disabled,
>>>>> it's unusable in this case.
>>>> Yes, you are right for current status. Because COLO best practices is
>>> replication + colo live migration + colo proxy.
>>>> But doesn't mean it has to be done in all scenarios as I explanation in V1.
>>>> The better way is allow to use x-colo capability firstly, and separate
>>>> this patch with two config options: --disable-replication  and --disable-x-
>>> colo.
>>> But what for? We for sure don't have such scenarios now (COLO without
>>> replication), as it's not allowed by far 7e934f5b27eee1b0d7 (by you and
>>> David).
>>>
>>> If you think we need such scenario, I think it should be a separate series
>>> which reverts 7e934f5b27eee1b0d7 and adds corresponding test and
>>> probably documentation.
>> In the patch 7e934f5b27eee1b0d7 said it's for current independent disk mode,
>> And what we talked about before is the shared disk mode.
>> Rethink about the COLO shared disk mode, this feature still needs some enabling works.
>> It looks OK for now and separate the build options when enabling COLO shared disk mode.
>
> I've started working on this, and now I see, that check in the migrate_caps_check() is not the only place.
>
> migration/colo.c has also several abort() points. For example,
> colo_process_checkpoint will simply abort if CONFIG_REPLICATION not
> defined.
>
> So for sure, current code is not prepared to use COLO with REPLICATION disabled.
>
> If this possibility is needed it requires more work. Personally, I
> don't think that possibility to enable COLO with disabled REPLICATION
> is really needed and I know nobody who need it, so that seems to be
> extra work.

Whoever does the work to make COLO without REPLICATION work, it can also
do the aditional work of splitting it.  Changing a configure file is
going to be the smaller of its problems.

Later, Juan.

[PATCH v2 4/4] configure: add --disable-colo-filters option

[Vladimir Sementsov-Ogievskiy]

Add option to not build COLO Proxy subsystem if it is not needed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 meson.build                   |  1 +
 meson_options.txt             |  2 ++
 net/meson.build               | 11 ++++++++---
 scripts/meson-buildoptions.sh |  3 +++
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/meson.build b/meson.build
index c44d05a13f..5b2fdfbd3a 100644
--- a/meson.build
+++ b/meson.build
@@ -1962,6 +1962,7 @@ config_host_data.set('CONFIG_GPROF', get_option('gprof'))
 config_host_data.set('CONFIG_LIVE_BLOCK_MIGRATION', get_option('live_block_migration').allowed())
 config_host_data.set('CONFIG_QOM_CAST_DEBUG', get_option('qom_cast_debug'))
 config_host_data.set('CONFIG_REPLICATION', get_option('replication').allowed())
+config_host_data.set('CONFIG_COLO_FILTERS', get_option('colo_filters').allowed())
 
 # has_header
 config_host_data.set('CONFIG_EPOLL', cc.has_header('sys/epoll.h'))
diff --git a/meson_options.txt b/meson_options.txt
index fc9447d267..ffe81317cb 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -289,6 +289,8 @@ option('live_block_migration', type: 'feature', value: 'auto',
        description: 'block migration in the main migration stream')
 option('replication', type: 'feature', value: 'auto',
        description: 'replication support')
+option('colo_filters', type: 'feature', value: 'auto',
+       description: 'colo_filters support')
 option('bochs', type: 'feature', value: 'auto',
        description: 'bochs image format support')
 option('cloop', type: 'feature', value: 'auto',
diff --git a/net/meson.build b/net/meson.build
index 38d50b8c96..7e54744aea 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -1,12 +1,9 @@
 softmmu_ss.add(files(
   'announce.c',
   'checksum.c',
-  'colo.c',
   'dump.c',
   'eth.c',
   'filter-buffer.c',
-  'filter-mirror.c',
-  'filter-rewriter.c',
   'filter.c',
   'hub.c',
   'net-hmp-cmds.c',
@@ -22,6 +19,14 @@ if get_option('replication').allowed()
   softmmu_ss.add(files('colo-compare.c'))
 endif
 
+if get_option('replication').allowed() or get_option('colo_filters').allowed()
+  softmmu_ss.add(files('colo.c'))
+endif
+
+if get_option('colo_filters').allowed()
+  softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c'))
+endif
+
 softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
 
 if have_l2tpv3
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 009fab1515..cf9d23369f 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -83,6 +83,7 @@ meson_options_help() {
   printf "%s\n" '  capstone        Whether and how to find the capstone library'
   printf "%s\n" '  cloop           cloop image format support'
   printf "%s\n" '  cocoa           Cocoa user interface (macOS only)'
+  printf "%s\n" '  colo-filters    colo_filters support'
   printf "%s\n" '  coreaudio       CoreAudio sound support'
   printf "%s\n" '  crypto-afalg    Linux AF_ALG crypto backend driver'
   printf "%s\n" '  curl            CURL block device driver'
@@ -236,6 +237,8 @@ _meson_option_parse() {
     --disable-cloop) printf "%s" -Dcloop=disabled ;;
     --enable-cocoa) printf "%s" -Dcocoa=enabled ;;
     --disable-cocoa) printf "%s" -Dcocoa=disabled ;;
+    --enable-colo-filters) printf "%s" -Dcolo_filters=enabled ;;
+    --disable-colo-filters) printf "%s" -Dcolo_filters=disabled ;;
     --enable-coreaudio) printf "%s" -Dcoreaudio=enabled ;;
     --disable-coreaudio) printf "%s" -Dcoreaudio=disabled ;;
     --enable-coroutine-pool) printf "%s" -Dcoroutine_pool=true ;;
-- 
2.34.1

RE: [PATCH v2 4/4] configure: add --disable-colo-filters option

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Thursday, April 20, 2023 6:53 AM
> To: qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy <vsementsov@yandex-
> team.ru>
> Subject: [PATCH v2 4/4] configure: add --disable-colo-filters option
> 
> Add option to not build COLO Proxy subsystem if it is not needed.

I think no need to add the --disable-colo-filter option.
Net-filters just general infrastructure. Another example is COLO also
use the -chardev socket to connect each filters. No need to add the --disable-colo-chardev....
Please drop this patch. 
But for COLO network part, still something need to do:
You can add --disable-colo-proxy not to build the net/colo-compare.c  if it is not needed.
This file just for COLO and not belong to network filters.

Thanks
Chen

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  meson.build                   |  1 +
>  meson_options.txt             |  2 ++
>  net/meson.build               | 11 ++++++++---
>  scripts/meson-buildoptions.sh |  3 +++
>  4 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index c44d05a13f..5b2fdfbd3a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1962,6 +1962,7 @@ config_host_data.set('CONFIG_GPROF',
> get_option('gprof'))
> config_host_data.set('CONFIG_LIVE_BLOCK_MIGRATION',
> get_option('live_block_migration').allowed())
>  config_host_data.set('CONFIG_QOM_CAST_DEBUG',
> get_option('qom_cast_debug'))
> config_host_data.set('CONFIG_REPLICATION',
> get_option('replication').allowed())
> +config_host_data.set('CONFIG_COLO_FILTERS',
> +get_option('colo_filters').allowed())
> 
>  # has_header
>  config_host_data.set('CONFIG_EPOLL', cc.has_header('sys/epoll.h')) diff --git
> a/meson_options.txt b/meson_options.txt index fc9447d267..ffe81317cb
> 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -289,6 +289,8 @@ option('live_block_migration', type: 'feature', value:
> 'auto',
>         description: 'block migration in the main migration stream')
> option('replication', type: 'feature', value: 'auto',
>         description: 'replication support')
> +option('colo_filters', type: 'feature', value: 'auto',
> +       description: 'colo_filters support')
>  option('bochs', type: 'feature', value: 'auto',
>         description: 'bochs image format support')  option('cloop', type: 'feature',
> value: 'auto', diff --git a/net/meson.build b/net/meson.build index
> 38d50b8c96..7e54744aea 100644
> --- a/net/meson.build
> +++ b/net/meson.build
> @@ -1,12 +1,9 @@
>  softmmu_ss.add(files(
>    'announce.c',
>    'checksum.c',
> -  'colo.c',
>    'dump.c',
>    'eth.c',
>    'filter-buffer.c',
> -  'filter-mirror.c',
> -  'filter-rewriter.c',
>    'filter.c',
>    'hub.c',
>    'net-hmp-cmds.c',
> @@ -22,6 +19,14 @@ if get_option('replication').allowed()
>    softmmu_ss.add(files('colo-compare.c'))
>  endif
> 
> +if get_option('replication').allowed() or
> +get_option('colo_filters').allowed()
> +  softmmu_ss.add(files('colo.c'))
> +endif
> +
> +if get_option('colo_filters').allowed()
> +  softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c')) endif
> +
>  softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
> 
>  if have_l2tpv3
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index 009fab1515..cf9d23369f 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -83,6 +83,7 @@ meson_options_help() {
>    printf "%s\n" '  capstone        Whether and how to find the capstone library'
>    printf "%s\n" '  cloop           cloop image format support'
>    printf "%s\n" '  cocoa           Cocoa user interface (macOS only)'
> +  printf "%s\n" '  colo-filters    colo_filters support'
>    printf "%s\n" '  coreaudio       CoreAudio sound support'
>    printf "%s\n" '  crypto-afalg    Linux AF_ALG crypto backend driver'
>    printf "%s\n" '  curl            CURL block device driver'
> @@ -236,6 +237,8 @@ _meson_option_parse() {
>      --disable-cloop) printf "%s" -Dcloop=disabled ;;
>      --enable-cocoa) printf "%s" -Dcocoa=enabled ;;
>      --disable-cocoa) printf "%s" -Dcocoa=disabled ;;
> +    --enable-colo-filters) printf "%s" -Dcolo_filters=enabled ;;
> +    --disable-colo-filters) printf "%s" -Dcolo_filters=disabled ;;
>      --enable-coreaudio) printf "%s" -Dcoreaudio=enabled ;;
>      --disable-coreaudio) printf "%s" -Dcoreaudio=disabled ;;
>      --enable-coroutine-pool) printf "%s" -Dcoroutine_pool=true ;;
> --
> 2.34.1

Re: [PATCH v2 4/4] configure: add --disable-colo-filters option

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 5586 bytes --]

On Thu, 20 Apr 2023 09:09:48 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Sent: Thursday, April 20, 2023 6:53 AM
> > To: qemu-devel@nongnu.org
> > Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
> > eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
> > Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> > thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
> > pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> > kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
> > lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy <vsementsov@yandex-  
> > team.ru>  
> > Subject: [PATCH v2 4/4] configure: add --disable-colo-filters option
> > 
> > Add option to not build COLO Proxy subsystem if it is not needed.  
> 
> I think no need to add the --disable-colo-filter option.
> Net-filters just general infrastructure. Another example is COLO also
> use the -chardev socket to connect each filters. No need to add the --disable-colo-chardev....
> Please drop this patch. 
> But for COLO network part, still something need to do:
> You can add --disable-colo-proxy not to build the net/colo-compare.c  if it is not needed.
> This file just for COLO and not belong to network filters.

And net/filter-rewriter.c is just for COLO too.

So in summary just drop net/filter-mirror.c from this patch?

> 
> Thanks
> Chen
> 
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >  meson.build                   |  1 +
> >  meson_options.txt             |  2 ++
> >  net/meson.build               | 11 ++++++++---
> >  scripts/meson-buildoptions.sh |  3 +++
> >  4 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index c44d05a13f..5b2fdfbd3a 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1962,6 +1962,7 @@ config_host_data.set('CONFIG_GPROF',
> > get_option('gprof'))
> > config_host_data.set('CONFIG_LIVE_BLOCK_MIGRATION',
> > get_option('live_block_migration').allowed())
> >  config_host_data.set('CONFIG_QOM_CAST_DEBUG',
> > get_option('qom_cast_debug'))
> > config_host_data.set('CONFIG_REPLICATION',
> > get_option('replication').allowed())
> > +config_host_data.set('CONFIG_COLO_FILTERS',
> > +get_option('colo_filters').allowed())
> > 
> >  # has_header
> >  config_host_data.set('CONFIG_EPOLL', cc.has_header('sys/epoll.h')) diff --git
> > a/meson_options.txt b/meson_options.txt index fc9447d267..ffe81317cb
> > 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -289,6 +289,8 @@ option('live_block_migration', type: 'feature', value:
> > 'auto',
> >         description: 'block migration in the main migration stream')
> > option('replication', type: 'feature', value: 'auto',
> >         description: 'replication support')
> > +option('colo_filters', type: 'feature', value: 'auto',
> > +       description: 'colo_filters support')
> >  option('bochs', type: 'feature', value: 'auto',
> >         description: 'bochs image format support')  option('cloop', type: 'feature',
> > value: 'auto', diff --git a/net/meson.build b/net/meson.build index
> > 38d50b8c96..7e54744aea 100644
> > --- a/net/meson.build
> > +++ b/net/meson.build
> > @@ -1,12 +1,9 @@
> >  softmmu_ss.add(files(
> >    'announce.c',
> >    'checksum.c',
> > -  'colo.c',
> >    'dump.c',
> >    'eth.c',
> >    'filter-buffer.c',
> > -  'filter-mirror.c',
> > -  'filter-rewriter.c',
> >    'filter.c',
> >    'hub.c',
> >    'net-hmp-cmds.c',
> > @@ -22,6 +19,14 @@ if get_option('replication').allowed()
> >    softmmu_ss.add(files('colo-compare.c'))
> >  endif
> > 
> > +if get_option('replication').allowed() or
> > +get_option('colo_filters').allowed()
> > +  softmmu_ss.add(files('colo.c'))
> > +endif
> > +
> > +if get_option('colo_filters').allowed()
> > +  softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c')) endif
> > +
> >  softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
> > 
> >  if have_l2tpv3
> > diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> > index 009fab1515..cf9d23369f 100644
> > --- a/scripts/meson-buildoptions.sh
> > +++ b/scripts/meson-buildoptions.sh
> > @@ -83,6 +83,7 @@ meson_options_help() {
> >    printf "%s\n" '  capstone        Whether and how to find the capstone library'
> >    printf "%s\n" '  cloop           cloop image format support'
> >    printf "%s\n" '  cocoa           Cocoa user interface (macOS only)'
> > +  printf "%s\n" '  colo-filters    colo_filters support'
> >    printf "%s\n" '  coreaudio       CoreAudio sound support'
> >    printf "%s\n" '  crypto-afalg    Linux AF_ALG crypto backend driver'
> >    printf "%s\n" '  curl            CURL block device driver'
> > @@ -236,6 +237,8 @@ _meson_option_parse() {
> >      --disable-cloop) printf "%s" -Dcloop=disabled ;;
> >      --enable-cocoa) printf "%s" -Dcocoa=enabled ;;
> >      --disable-cocoa) printf "%s" -Dcocoa=disabled ;;
> > +    --enable-colo-filters) printf "%s" -Dcolo_filters=enabled ;;
> > +    --disable-colo-filters) printf "%s" -Dcolo_filters=disabled ;;
> >      --enable-coreaudio) printf "%s" -Dcoreaudio=enabled ;;
> >      --disable-coreaudio) printf "%s" -Dcoreaudio=disabled ;;
> >      --enable-coroutine-pool) printf "%s" -Dcoroutine_pool=true ;;
> > --
> > 2.34.1  
> 
> 



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/4] configure: add --disable-colo-filters option

[Vladimir Sementsov-Ogievskiy]

On 20.04.23 12:09, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Thursday, April 20, 2023 6:53 AM
>> To: qemu-devel@nongnu.org
>> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
>> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
>> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
>> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
>> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
>> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
>> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy <vsementsov@yandex-
>> team.ru>
>> Subject: [PATCH v2 4/4] configure: add --disable-colo-filters option
>>
>> Add option to not build COLO Proxy subsystem if it is not needed.
> 
> I think no need to add the --disable-colo-filter option.
> Net-filters just general infrastructure. Another example is COLO also
> use the -chardev socket to connect each filters. No need to add the --disable-colo-chardev....
> Please drop this patch.

I don't follow your reasoning. Of course, we don't need any option like this, and can simply include to build all the components we don't use. So "no need" is correct for any --disable-* option.
Still, I think, it's good, when you can exclude from build the subsystems that you don't need / don't want to maintain or ship to your end users.

In MAINTAINERS these two filters are in "COLO Proxy" susbsystem. Is it correct? What's wrong with adding an option to not build this subsystem? I can rename it to --disable-colo-proxy.

> But for COLO network part, still something need to do:
> You can add --disable-colo-proxy not to build the net/colo-compare.c  if it is not needed.
> This file just for COLO and not belong to network filters.

net/colo-compare.c is used only only for COLO, which in turn used only with CONFIG_REPLICATION enabled, see patch 3. So, no reason to add separate option for it, it should be disabled with --disable-replication.

-- 
Best regards,
Vladimir

RE: [PATCH v2 4/4] configure: add --disable-colo-filters option

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Thursday, April 20, 2023 7:26 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> kwolf@redhat.com; lizhijian@fujitsu.com
> Subject: Re: [PATCH v2 4/4] configure: add --disable-colo-filters option
> 
> On 20.04.23 12:09, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> Sent: Thursday, April 20, 2023 6:53 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: qemu-block@nongnu.org; michael.roth@amd.com;
> armbru@redhat.com;
> >> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com;
> Zhang,
> >> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> >> thuth@redhat.com; berrange@redhat.com;
> marcandre.lureau@redhat.com;
> >> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> >> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
> >> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy
> >> <vsementsov@yandex- team.ru>
> >> Subject: [PATCH v2 4/4] configure: add --disable-colo-filters option
> >>
> >> Add option to not build COLO Proxy subsystem if it is not needed.
> >
> > I think no need to add the --disable-colo-filter option.
> > Net-filters just general infrastructure. Another example is COLO also
> > use the -chardev socket to connect each filters. No need to add the --
> disable-colo-chardev....
> > Please drop this patch.
> 
> I don't follow your reasoning. Of course, we don't need any option like this,
> and can simply include to build all the components we don't use. So "no
> need" is correct for any --disable-* option.
> Still, I think, it's good, when you can exclude from build the subsystems that
> you don't need / don't want to maintain or ship to your end users.

Yes, I agree with your idea.

> 
> In MAINTAINERS these two filters are in "COLO Proxy" susbsystem. Is it
> correct? What's wrong with adding an option to not build this subsystem? I
> can rename it to --disable-colo-proxy.

The history is COLO project contributed QEMU filter framework and filter-mirror/redirector...etc..
And the "COLO Proxy" susbsystem covered the filters do not means it belong to COLO.
So, It is unreasonable to tell users that you cannot use filter-mirror/rediretor for network debugging
Or other purpose because you have not enabled COLO proxy.

> 
> > But for COLO network part, still something need to do:
> > You can add --disable-colo-proxy not to build the net/colo-compare.c  if it is
> not needed.
> > This file just for COLO and not belong to network filters.
> 
> net/colo-compare.c is used only only for COLO, which in turn used only with
> CONFIG_REPLICATION enabled, see patch 3. So, no reason to add separate
> option for it, it should be disabled with --disable-replication.

Yes, and as Lukas said, COLO is the only user for the filter-rewriter currently.
It is more appropriate to add filter-rewriter replace the filter-mirror here.
I saw the patch 3, it is better to move it to this patch.

Thanks
Chen

> 
> --
> Best regards,
> Vladimir

Re: [PATCH v2 4/4] configure: add --disable-colo-filters option

[Vladimir Sementsov-Ogievskiy]

On 21.04.23 05:22, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Sent: Thursday, April 20, 2023 7:26 PM
>> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
>> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
>> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
>> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
>> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
>> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
>> kwolf@redhat.com; lizhijian@fujitsu.com
>> Subject: Re: [PATCH v2 4/4] configure: add --disable-colo-filters option
>>
>> On 20.04.23 12:09, Zhang, Chen wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> Sent: Thursday, April 20, 2023 6:53 AM
>>>> To: qemu-devel@nongnu.org
>>>> Cc: qemu-block@nongnu.org; michael.roth@amd.com;
>> armbru@redhat.com;
>>>> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com;
>> Zhang,
>>>> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
>>>> thuth@redhat.com; berrange@redhat.com;
>> marcandre.lureau@redhat.com;
>>>> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
>>>> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
>>>> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy
>>>> <vsementsov@yandex- team.ru>
>>>> Subject: [PATCH v2 4/4] configure: add --disable-colo-filters option
>>>>
>>>> Add option to not build COLO Proxy subsystem if it is not needed.
>>>
>>> I think no need to add the --disable-colo-filter option.
>>> Net-filters just general infrastructure. Another example is COLO also
>>> use the -chardev socket to connect each filters. No need to add the --
>> disable-colo-chardev....
>>> Please drop this patch.
>>
>> I don't follow your reasoning. Of course, we don't need any option like this,
>> and can simply include to build all the components we don't use. So "no
>> need" is correct for any --disable-* option.
>> Still, I think, it's good, when you can exclude from build the subsystems that
>> you don't need / don't want to maintain or ship to your end users.
> 
> Yes, I agree with your idea.
> 
>>
>> In MAINTAINERS these two filters are in "COLO Proxy" susbsystem. Is it
>> correct? What's wrong with adding an option to not build this subsystem? I
>> can rename it to --disable-colo-proxy.
> 
> The history is COLO project contributed QEMU filter framework and filter-mirror/redirector...etc..
> And the "COLO Proxy" susbsystem covered the filters do not means it belong to COLO.
> So, It is unreasonable to tell users that you cannot use filter-mirror/rediretor for network debugging
> Or other purpose because you have not enabled COLO proxy.

But we don't say it to users, as these filters are enabled by default.

But I see your point. And looking at filter-mirror.c I see that there is no relations with colo. Can't say this about filter-rewriter.c

So, absolutely correct would be just have separate options

--disable-net-filter-mirror
--disable-net-filter-rewriter

and for any other filter we want to be "disable-able", like options for block drivers (I mean --disable-parallels, --disable-qcow1, --disable-qed, etc for files describing these drivers in block/)


> 
>>
>>> But for COLO network part, still something need to do:
>>> You can add --disable-colo-proxy not to build the net/colo-compare.c  if it is
>> not needed.
>>> This file just for COLO and not belong to network filters.
>>
>> net/colo-compare.c is used only only for COLO, which in turn used only with
>> CONFIG_REPLICATION enabled, see patch 3. So, no reason to add separate
>> option for it, it should be disabled with --disable-replication.
> 
> Yes, and as Lukas said, COLO is the only user for the filter-rewriter currently.

So, maybe simply move filter-rewriter.c under CONFIG_REPLICATION, if it's needed only for COLO?

> It is more appropriate to add filter-rewriter replace the filter-mirror here.
> I saw the patch 3, it is better to move it to this patch.

Hmm what do you mean? Both filter-rewriter and filter-mirror are now handled in this patch, so what to replace?

> 
> Thanks
> Chen
> 
>>
>> --
>> Best regards,
>> Vladimir
> 

-- 
Best regards,
Vladimir

RE: [PATCH v2 4/4] configure: add --disable-colo-filters option

[Zhang, Chen]

> -----Original Message-----
> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Sent: Friday, April 21, 2023 4:53 PM
> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; michael.roth@amd.com; armbru@redhat.com;
> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com; Zhang,
> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> thuth@redhat.com; berrange@redhat.com; marcandre.lureau@redhat.com;
> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> kwolf@redhat.com; lizhijian@fujitsu.com
> Subject: Re: [PATCH v2 4/4] configure: add --disable-colo-filters option
> 
> On 21.04.23 05:22, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> Sent: Thursday, April 20, 2023 7:26 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org
> >> Cc: qemu-block@nongnu.org; michael.roth@amd.com;
> armbru@redhat.com;
> >> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com;
> Zhang,
> >> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> >> thuth@redhat.com; berrange@redhat.com;
> marcandre.lureau@redhat.com;
> >> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> >> kwolf@redhat.com; lizhijian@fujitsu.com
> >> Subject: Re: [PATCH v2 4/4] configure: add --disable-colo-filters
> >> option
> >>
> >> On 20.04.23 12:09, Zhang, Chen wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >>>> Sent: Thursday, April 20, 2023 6:53 AM
> >>>> To: qemu-devel@nongnu.org
> >>>> Cc: qemu-block@nongnu.org; michael.roth@amd.com;
> >> armbru@redhat.com;
> >>>> eblake@redhat.com; jasowang@redhat.com; quintela@redhat.com;
> >> Zhang,
> >>>> Hailiang <zhanghailiang@xfusion.com>; philmd@linaro.org;
> >>>> thuth@redhat.com; berrange@redhat.com;
> >> marcandre.lureau@redhat.com;
> >>>> pbonzini@redhat.com; dave@treblig.org; hreitz@redhat.com;
> >>>> kwolf@redhat.com; Zhang, Chen <chen.zhang@intel.com>;
> >>>> lizhijian@fujitsu.com; Vladimir Sementsov-Ogievskiy
> >>>> <vsementsov@yandex- team.ru>
> >>>> Subject: [PATCH v2 4/4] configure: add --disable-colo-filters
> >>>> option
> >>>>
> >>>> Add option to not build COLO Proxy subsystem if it is not needed.
> >>>
> >>> I think no need to add the --disable-colo-filter option.
> >>> Net-filters just general infrastructure. Another example is COLO
> >>> also use the -chardev socket to connect each filters. No need to add
> >>> the --
> >> disable-colo-chardev....
> >>> Please drop this patch.
> >>
> >> I don't follow your reasoning. Of course, we don't need any option
> >> like this, and can simply include to build all the components we
> >> don't use. So "no need" is correct for any --disable-* option.
> >> Still, I think, it's good, when you can exclude from build the
> >> subsystems that you don't need / don't want to maintain or ship to your
> end users.
> >
> > Yes, I agree with your idea.
> >
> >>
> >> In MAINTAINERS these two filters are in "COLO Proxy" susbsystem. Is
> >> it correct? What's wrong with adding an option to not build this
> >> subsystem? I can rename it to --disable-colo-proxy.
> >
> > The history is COLO project contributed QEMU filter framework and filter-
> mirror/redirector...etc..
> > And the "COLO Proxy" susbsystem covered the filters do not means it
> belong to COLO.
> > So, It is unreasonable to tell users that you cannot use
> > filter-mirror/rediretor for network debugging Or other purpose because
> you have not enabled COLO proxy.
> 
> But we don't say it to users, as these filters are enabled by default.
> 
> But I see your point. And looking at filter-mirror.c I see that there is no
> relations with colo. Can't say this about filter-rewriter.c
> 
> So, absolutely correct would be just have separate options
> 
> --disable-net-filter-mirror
> --disable-net-filter-rewriter
> 
> and for any other filter we want to be "disable-able", like options for block
> drivers (I mean --disable-parallels, --disable-qcow1, --disable-qed, etc for
> files describing these drivers in block/)
> 

Yes.

> 
> >
> >>
> >>> But for COLO network part, still something need to do:
> >>> You can add --disable-colo-proxy not to build the net/colo-compare.c
> >>> if it is
> >> not needed.
> >>> This file just for COLO and not belong to network filters.
> >>
> >> net/colo-compare.c is used only only for COLO, which in turn used
> >> only with CONFIG_REPLICATION enabled, see patch 3. So, no reason to
> >> add separate option for it, it should be disabled with --disable-replication.
> >
> > Yes, and as Lukas said, COLO is the only user for the filter-rewriter currently.
> 
> So, maybe simply move filter-rewriter.c under CONFIG_REPLICATION, if it's
> needed only for COLO?
> 

As I know, in QEMU side, COLO is the only user for filter-rewriter.
But QEMU user(libvirt...etc...) may try to use it for other proposal.

> > It is more appropriate to add filter-rewriter replace the filter-mirror here.
> > I saw the patch 3, it is better to move it to this patch.
> 
> Hmm what do you mean? Both filter-rewriter and filter-mirror are now
> handled in this patch, so what to replace?

I means it's better to make the net/colo-compare.c and net/filter-rewriter.c to this patch for
The option: --disable-colo-proxy

Thanks
Chen

> 
> >
> > Thanks
> > Chen
> >
> >>
> >> --
> >> Best regards,
> >> Vladimir
> >
> 
> --
> Best regards,
> Vladimir

Re: [PATCH v2 0/4] COLO: improve build options

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On Thu, 20 Apr 2023 01:52:28 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:

> Hi all!
> 
> COLO substem seems to be useless when CONFIG_REPLICATION is unset, as we
> simply don't allow to set x-colo capability in this case. So, let's not
> compile in unreachable code and interface we cannot use when
> CONFIG_REPLICATION is unset.
> 
> Also, provide personal configure option for COLO Proxy subsystem.
> 
> v1 was 
> [PATCH] replication: compile out some staff when replication is not configured
> Supersedes: <20230411145112.497785-1-vsementsov@yandex-team.ru>

Hey,
This series is a good idea, and looks fine to me. Maybe you can remove
the #ifdef CONFIG_REPLICATION/#ifndef CONFIG_REPLICATION from
migration/colo.c too while you are at it.

Regards,
Lukas Straub

> Vladimir Sementsov-Ogievskiy (4):
>   block/meson.build: prefer positive condition for replication
>   scripts/qapi: allow optional experimental enum values
>   build: move COLO under CONFIG_REPLICATION
>   configure: add --disable-colo-filters option
> 
>  block/meson.build              |  2 +-
>  hmp-commands.hx                |  2 ++
>  meson.build                    |  1 +
>  meson_options.txt              |  2 ++
>  migration/colo.c               |  6 +++++
>  migration/meson.build          |  6 +++--
>  migration/migration-hmp-cmds.c |  2 ++
>  migration/migration.c          | 19 +++-----------
>  net/meson.build                | 16 +++++++++---
>  qapi/migration.json            | 12 ++++++---
>  scripts/meson-buildoptions.sh  |  3 +++
>  scripts/qapi/types.py          |  2 ++
>  stubs/colo.c                   | 47 ++++++++++++++++++++++++++++++++++
>  stubs/meson.build              |  1 +
>  14 files changed, 95 insertions(+), 26 deletions(-)
>  create mode 100644 stubs/colo.c
> 



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] COLO: improve build options

[Vladimir Sementsov-Ogievskiy]

On 20.04.23 11:33, Lukas Straub wrote:
> On Thu, 20 Apr 2023 01:52:28 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> 
>> Hi all!
>>
>> COLO substem seems to be useless when CONFIG_REPLICATION is unset, as we
>> simply don't allow to set x-colo capability in this case. So, let's not
>> compile in unreachable code and interface we cannot use when
>> CONFIG_REPLICATION is unset.
>>
>> Also, provide personal configure option for COLO Proxy subsystem.
>>
>> v1 was
>> [PATCH] replication: compile out some staff when replication is not configured
>> Supersedes: <20230411145112.497785-1-vsementsov@yandex-team.ru>
> 
> Hey,
> This series is a good idea, and looks fine to me. Maybe you can remove
> the #ifdef CONFIG_REPLICATION/#ifndef CONFIG_REPLICATION from
> migration/colo.c too while you are at it.
> 

Oh, right, will do in v3. Thanks!

-- 
Best regards,
Vladimir