Re: [PATCH net-next v3 1/8] docs: add more netlink docs (incl. spec docs)

[Jakub Kicinski <kuba@kernel.org>]

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?

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

> 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.

> > +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 :(

> > +checks
> > +------
> > +
> > +Documentation for the ``checks`` sub-sections of attribute specs.
> > +
> > +unterminated-ok
> > +~~~~~~~~~~~~~~~
> > +
> > +Accept strings without the null-termination (for legacy families only).
> > +Switches from the ``NLA_NUL_STRING`` to ``NLA_STRING`` policy type.  
> 
> Should we even document all the legacy bits in such a prominent place?
> 
> (or just move it after max-len/min-len?)

This is kernel-only info (checks) so I figured it's good enough until
someone takes a more serious stab at supporting legacy families.

> > +Attribute type nests
> > +--------------------
> > +
> > +New Netlink families should use ``multi-attr`` to define arrays.  
> 
> 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 :(

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.