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