From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Machata Subject: Re: [PATCH net-next 2/2] net: bridge: Notify about !added_by_user FDB entries Date: Wed, 02 May 2018 01:33:29 +0300 Message-ID: References: <8934024270c83055e1e0e9468aafa3fe5e35e745.1525194039.git.petrm@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain Cc: netdev@vger.kernel.org, ivecera@redhat.com, davem@davemloft.net, stephen@networkplumber.org, andrew@lunn.ch, vivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com, jiri@resnulli.us, "bridge\@lists.linux-foundation.org" To: Nikolay Aleksandrov Return-path: Received: from mail-eopbgr30074.outbound.protection.outlook.com ([40.107.3.74]:15342 "EHLO EUR03-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751369AbeEAWdx (ORCPT ); Tue, 1 May 2018 18:33:53 -0400 In-Reply-To: (Nikolay Aleksandrov's message of "Tue, 1 May 2018 20:49:15 +0300") Sender: netdev-owner@vger.kernel.org List-ID: Nikolay Aleksandrov writes: > On 01/05/18 20:04, Petr Machata wrote: >> Do not automatically bail out on sending notifications about activity on >> non-user-added FDB entries. Instead, notify about this activity except >> for cases where the activity itself originates in a notification, to >> avoid sending duplicate notifications. >> >> Signed-off-by: Petr Machata >> --- >> net/bridge/br.c | 4 ++-- >> net/bridge/br_fdb.c | 40 +++++++++++++++++++++++++++------------- >> net/bridge/br_private.h | 4 ++-- >> net/bridge/br_switchdev.c | 2 +- >> 4 files changed, 32 insertions(+), 18 deletions(-) >> > > Hi Petr, > We already have 7 different fdb delete functions, I'm really not a fan of > adding yet another one for such trivial change. > Why don't you just add the new notify parameter to the already existing > fdb_delete() ? (actually about the name see below) > IMO it's confusing - if one wants a notification then use fdb_delete() or __fdb_delete(true) > vs __fdb_delete(false) if a notification is not required. I think simply having the last > parameter everywhere for fdb_delete() shows the intention clearer and avoids another > fdb delete function. All right--this is how I had it written actually, but then decided to do this wrapping, because so many of the calls end up being true. I'll send a v2 with just the extra argument. > Another point, the notify parameter has a confusing name in this context because > you're controlling the switchdev notifications not the rtnetlink ones. I'd suggest > changing the name to something more descriptive like swdev_notify, otherwise you > could get the funny end result of calling __fdb_notify() with notify == false which > to me means don't notify. :-) OK, swdev_notify it will be. > Also please add the bridge maintainers to the CC list. bridge@lists.linux-foundation.org? I saw it's a moderated list and for some reason that made me think it's not meant for patch postings. I'll add them the next time. Thanks, Petr