On Thu, 19 Jan 2023 21:29:22 +0100 Johannes Berg wrote:

On Wed, 2023-01-18 at 16:36 -0800, Jakub Kicinski wrote:

quoted

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

quoted

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

quoted

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

quoted

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