On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:

On 18/04/18 15:07, Joachim Nilsson wrote:

quoted

- First of all, is this patch useful to anyone

Obviously to us as it's based on our patch. :-)
We actually recently discussed what will be needed to make it acceptable to upstream.

Great! :)

quoted

- The current br_multicast.c is very complex.  The support for both IPv4
   and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and
   'br->vlan_enabled' ... this has likely been discussed before, but if
   we could remove those code paths I believe what's left would be quite
   a bit easier to read and maintain.

br->vlan_enabled has a wrapper that can be used without ifdefs, as does br_vlan_find()
so in short - you can remove the ifdefs and use the wrappers,  they'll degrade to always
false/null when vlans are disabled.

Thanks, I'll have a look at that and prepare an RFC v2!

quoted

- Many per-bridge specific multicast sysfs settings may need to have a
   corresponding per-VLAN setting, e.g. snooping, query_interval, etc.
   How should we go about that? (For status reporting I have a proposal)

We'll have to add more to the per-vlan context, but yes it has to happen.
It will be only netlink interface for config/retrieval, no sysfs.

Some settings are possible to do with sysfs, like multicast_query_interval
and ...

quoted

- Dito per-port specific multicast sysfs settings, e.g. multicast_router

I'm not sure I follow this one, there is per-port mcast router config now ?

Sorry no, I meant we may want to add more per-VLAN settings when we get
this base patch merged.  Like router ports, we may want to be able to
set them per VLAN.

Thanks for the effort, I see that you have done some of the required cleanups
for this to be upstreamable, but as you've noted above we need to make it
complete (with the per-vlan contexts and all).

There's definitely more work to be done.  Agreeing on a base set of changes
to start with is maybe the most important, as well as making it complete.

I will review this patch in detail later and come back if there's anything.

Thank you so much for the quick feedback so far! :)

Cheers
 /Joachim