Message-ID: <2b7f7f76aac4fcf2a51eb5588e64316b62f27d65.camel@sipsolutions.net>
Date:   Fri, 20 Jan 2023 10:15:39 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, robh@...nel.org, stephen@...workplumber.org,
        ecree.xilinx@...il.com, sdf@...gle.com, f.fainelli@...il.com,
        fw@...len.de, linux-doc@...r.kernel.org, razor@...ckwall.org,
        nicolas.dichtel@...nd.com, Bagas Sanjaya <bagasdotme@...il.com>
Subject: Re: [PATCH net-next v3 1/8] docs: add more netlink docs (incl. spec
 docs)

On Thu, 2023-01-19 at 18:13 -0800, Jakub Kicinski wrote:
> On Thu, 19 Jan 2023 21:29:22 +0100 Johannes Berg wrote:
> > On Wed, 2023-01-18 at 16:36 -0800, Jakub Kicinski wrote:
> > > 
> > > +Answer requests
> > > +---------------
> > > +
> > > +Older families do not reply to all of the commands, especially NEW / ADD
> > > +commands. User only gets information whether the operation succeeded or
> > > +not via the ACK. Try to find useful data to return. Once the command is
> > > +added whether it replies with a full message or only an ACK is uAPI and
> > > +cannot be changed. It's better to err on the side of replying.
> > > +
> > > +Specifically NEW and ADD commands should reply with information identifying
> > > +the created object such as the allocated object's ID (without having to
> > > +resort to using ``NLM_F_ECHO``).  
> > 
> > I'm a bit on the fence on this recommendation (as written).
> > 
> > Yeah, it's nice to reply to things ... but!
> > 
> > In userspace, you often request and wait for the ACK to see if the
> > operation succeeded. This is basically necessary. But then it's
> > complicated to wait for *another* message to see the ID.
> 
> Maybe you're looking at this from the perspective of a person tired 
> of manually writing the user space code?

Heh, I guess :)

> If the netlink message handling code is all auto-generated it makes 
> no difference to the user...

True. And I guess we can hope that'll be the case for most users, though
I doubt it :)

> > We've actually started using the "cookie" in the extack to report an ID
> > of an object/... back, see uses of nl_set_extack_cookie_u64() in the
> > tree.
> 
> ... and to the middle-layer / RPC / auto-generated library pulling
> stuff out from protocol messages and interpreting them in a special 
> way is a typical netlink vortex of magic :(

:)

> > So I'm not sure I wholeheartedly agree with the recommendation to send a
> > separate answer. We've done that, but it's ugly on both sender side in
> > the kernel (requiring an extra message allocation, ideally at the
> > beginning of the operation so you can fail gracefully, etc.) and on the
> > receiver (having to wait for another message if the operation was
> > successful; possibly actually having to check for that message *before*
> > the ACK arrives.)
> 
> Right, response is before ACK. It's not different to a GET command,
> really.

True.

> > > +Support dump consistency
> > > +------------------------
> > > +
> > > +If iterating over objects during dump may skip over objects or repeat
> > > +them - make sure to report dump inconsistency with ``NLM_F_DUMP_INTR``.  
> > 
> > That could be a bit more fleshed out on _how_ to do that, if it's not
> > somewhere else?
> 
> I was thinking about adding a sentence like "To avoid consistency
> issues store your objects in an Xarray and correctly use the ID during
> iteration".. but it seems to hand-wavy. Really the coder needs to
> understand dumps quite well to get what's going on, and then the
> consistency is kinda obvious. IDK. Almost nobody gets this right :(

Yeah agree, it's tricky one way or the other. To be honest I was
thinking less of documenting the mechanics of the underlying code to
ensure that, but rather of the mechanics of using the APIs to ensure
that, i.e. how to use cb->seq and friends.

> > Unrelated to this particular document, but ...
> > 
> > I'm all for this, btw, but maybe we should have a way of representing in
> > the policy that an attribute is used as multi-attr for an array, and a
> > way of exposing that in the policy export? Hmm. Haven't thought about
> > this for a while.
> 
> Informational-only or enforced? Enforcing this now would be another
> backward-compat nightmare :(

More informational - for userspace to know from policy dump that certain
attributes have that property. With nested it's easy to know (there's a
special nested-array type), but multi-attr there's no way to distinguish
"is this one" and "is this multiple".

Now ... you might say you don't really care now since you want
everything to be auto-generated and then you have it in the docs
(actually, do you?), and that's a fair point.

> FWIW I have a set parked on a branch to add "required" bit to policies,
> so for per-op policies one can reject requests with missing attrs
> during validation.

Nice. That might yet convince me of per-op policies ;-)

Though IMHO the namespace issue remains - I'd still not like to have 100
definitions of NL80211_ATTR_IFINDEX or similar.

johannes

Powered by blists - more mailing lists