Date:   Thu, 19 Jan 2023 21:29:22 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     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 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.

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.

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

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

> +kernel-policy
> +~~~~~~~~~~~~~
> +
> +Defines if the kernel validation policy is per operation (``per-op``)
> +or for the entire family (``global``). New families should use ``per-op``
> +(default) to be able to narrow down the attributes accepted by a specific
> +command.

Again I'm not sure I agree with that recommendation, but I know it's
your preference :-)

(IMHO some things become more complex, such as having a "ifindex" in
each one of them)

> +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?)

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

johannes

Powered by blists - more mailing lists