netdev - Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y3g6mpr2.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Sat, 26 May 2018 10:54:25 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Petr Machata <petrm@...lanox.com>
Cc:     netdev@...r.kernel.org, devel@...verdev.osuosl.org,
        bridge@...ts.linux-foundation.org, jiri@...lanox.com,
        idosch@...lanox.com, davem@...emloft.net,
        razvan.stefanescu@....com, gregkh@...uxfoundation.org,
        stephen@...workplumber.org, andrew@...n.ch, f.fainelli@...il.com,
        nikolay@...ulusnetworks.com
Subject: Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

Hi Petr,

Petr Machata <petrm@...lanox.com> writes:

> Vivien Didelot <vivien.didelot@...oirfairelinux.com> writes:
>
>>> +	} else {
>>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>> +		if (err && err != -EOPNOTSUPP)
>>> +			goto out;
>>>  	}
>>
>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>> seems confusing to me, the change looks good:
>
> I'm not sure what you're aiming at. Both VID and flags are sent with the
> notification, so they need to be passed on to the function somehow. Do
> you have a counterproposal for the API?

I'm only questioning the code organization here, not the functional
aspect which I do agree with. What I'm saying is that you name a new
switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
(vid and flags.) How would you call another eventual helper taking MDB
arguments, br_switchdev_port_OBJ_add again? So something like
br_switchdev_port_VLAN_add would be more intuitive.

At the same time there's an effort to centralize all switchdev helpers
of the bridge layer (i.e. the software -> hardware bridge calls) into
net/bridge/br_switchdev.c, so that file would be more adequate.

You may discard my comments but I think it'd be beneficial to us all to
finally keep a bit of consistency in that bridge layer code.

Thanks,

        Vivien