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.
