Date:   Thu, 22 Sep 2022 13:09:51 +0200
From:   Guillaume Nault <gnault@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Ido Schimmel <idosch@...dia.com>,
        Petr Machata <petrm@...dia.com>,
        Florent Fourcot <florent.fourcot@...irst.fr>,
        Nikolay Aleksandrov <razor@...ckwall.org>
Subject: Re: [PATCH net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new,
 set}link

On Wed, Sep 21, 2022 at 03:56:40PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Sep 2022 18:14:09 +0200 Guillaume Nault wrote:
> > > I'd love to hear what others think. IMO we should declare a moratorium
> > > on any use of netlink flags and fixed fields, push netlink towards
> > > being a simple conduit for TLVs.  
> > 
> > At my previous employer, we had a small program inserting and removing
> > routes depending on several external events (not a full-fledged routing
> > daemon). NLM_F_ECHO was used at least to log the real kernel actions (as
> > opposed to what the program intended to do) and link that to the events
> > that triggered these actions. That was really helpful for network
> > administrators. Yes, we were lucky that the RTM_NEWROUTE and
> > RTM_DELROUTE message handlers supported NLM_F_ECHO. I was surprised when
> > I later realised that RTM_NEWLINK and many others didn't.
> > 
> > Then, a few years ago, I had questions from another team (maybe Network
> > Manager but I'm not sure) who asked how to reliably retrieve
> > informations like the ifindex of newly created devices. That's the use
> > case NLM_F_ECHO is for, but lacking this feature this team had to
> > rely on a more convoluted and probably racy way. That was the moment
> > I decided to expose the problem to our team. Fast-forwarding a couple
> > of years and Hangbin picked up the task.
> 
> Looking closer at the code it seems like what NLM_F_ECHO does in most
> places is to loop notifications resulting from the command back onto
> the requesting socket. See nlmsg_notify(), report is usually passed 
> as nlmsg_report(req).

Yes, this is how it's supposed to work. NLM_F_ECHO is already handled
by the core netlink code. Netlink message handlers only have to pass
the right parameters to nlmsg_notify() (or the rtnl_notify() wrapper
for rtnetlink) to make it work.

That's why I complained when RTM_NEWNSID tried to implement its own
notification mechanism:
https://lore.kernel.org/netdev/20191003161940.GA31862@linux.home/

I mean, let's just use the built-in mechanism, rather than reinventing
a new one every time the need comes up.

> I guess that answers Hangbin's question - yes, I'd vote that we just
> pass the nlh to rtnl_notify() and let the netlink core do its thing.

Definitely.

> In general I still don't think NLM_F_ECHO makes for a reasonable API.
> It may seem okay to those who are willing to write manual netlink
> parsers but for a normal programmer the ability to receive directly
> notifications resulting from a API call they made is going to mean..
> nothing they can have prior experience with. NEWLINK should have
> reported the allocated handle / ifindex from the start :(

I also don't know of any other API that works this way. But given the
current situation, I also can't really see that change.

> The "give me back the notifications" semantics match well your use
> case to log what the command has done, in that case there is no need
> to "return" all the notifications from the API call.

It's not really _my_ use case anymore :), but I'm pretty sure this
piece of software is still in use. Anyway I used this example just to
illustrate why a programmer would use this feature. The RTM_NEWNSID
is another practical use case.

Powered by blists - more mailing lists