RE: [PATCH] replication: compile out some staff when replication is not configured

[Zhang, Chen]

> -----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
>