On Thu, 2023-01-19 at 18:13 -0800, Jakub Kicinski wrote:

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

quoted

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?

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 :)

quoted

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

:)

quoted

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.

quoted

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

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.

quoted

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