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