From: Jakub Kicinski <kuba@kernel•org>
To: Johannes Berg <johannes@sipsolutions•net>
Cc: davem@davemloft•net, netdev@vger•kernel.org, edumazet@google•com,
pabeni@redhat•com, robh@kernel•org, stephen@networkplumber•org,
ecree.xilinx@gmail•com, sdf@google•com, f.fainelli@gmail•com,
fw@strlen•de, linux-doc@vger•kernel.org, razor@blackwall•org,
nicolas.dichtel@6wind•com, Bagas Sanjaya <bagasdotme@gmail•com>
Subject: Re: [PATCH net-next v3 1/8] docs: add more netlink docs (incl. spec docs)
Date: Thu, 19 Jan 2023 18:13:06 -0800 [thread overview]
Message-ID: <20230119181306.3b8491b1@kernel.org> (raw)
In-Reply-To: <96618285a772b5ef9998f638ea17ff68c32dd710.camel@sipsolutions.net>
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.
next prev parent reply other threads:[~2023-01-20 4:45 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-19 0:36 [PATCH net-next v3 0/8] Netlink protocol specs Jakub Kicinski
2023-01-19 0:36 ` [PATCH net-next v3 1/8] docs: add more netlink docs (incl. spec docs) Jakub Kicinski
2023-01-19 15:48 ` Vladimir Oltean
2023-01-19 20:29 ` Johannes Berg
2023-01-20 0:23 ` Jacob Keller
2023-01-20 9:10 ` Johannes Berg
2023-01-20 18:35 ` Keller, Jacob E
2023-01-20 2:13 ` Jakub Kicinski [this message]
2023-01-20 9:15 ` Johannes Berg
2023-01-20 17:23 ` Jakub Kicinski
2023-01-19 0:36 ` [PATCH net-next v3 2/8] netlink: add schemas for YAML specs Jakub Kicinski
2023-01-19 14:07 ` Rob Herring
2023-01-19 21:49 ` Jakub Kicinski
2023-01-19 22:24 ` Jakub Kicinski
2023-01-19 23:02 ` Rob Herring
2023-01-20 0:08 ` Jakub Kicinski
2023-01-20 14:43 ` Rob Herring
2023-01-19 0:36 ` [PATCH net-next v3 3/8] net: add basic C code generators for Netlink Jakub Kicinski
2023-01-19 20:53 ` Johannes Berg
2023-01-20 1:53 ` Jakub Kicinski
2023-01-20 9:17 ` Johannes Berg
2023-01-19 0:36 ` [PATCH net-next v3 4/8] netlink: add a proto specification for FOU Jakub Kicinski
2023-01-19 0:36 ` [PATCH net-next v3 5/8] net: fou: regenerate the uAPI from the spec Jakub Kicinski
2023-01-19 0:36 ` [PATCH net-next v3 6/8] net: fou: rename the source for linking Jakub Kicinski
2023-01-19 0:36 ` [PATCH net-next v3 7/8] net: fou: use policy and operation tables generated from the spec Jakub Kicinski
2023-01-19 20:56 ` Johannes Berg
2023-01-20 0:18 ` Jacob Keller
2023-01-20 1:04 ` Jakub Kicinski
2023-01-19 0:36 ` [PATCH net-next v3 8/8] tools: ynl: add a completely generic client Jakub Kicinski
2023-01-20 0:50 ` Jacob Keller
2023-01-19 17:07 ` [PATCH net-next v3 0/8] Netlink protocol specs Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230119181306.3b8491b1@kernel.org \
--to=kuba@kernel$(echo .)org \
--cc=bagasdotme@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=ecree.xilinx@gmail$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=f.fainelli@gmail$(echo .)com \
--cc=fw@strlen$(echo .)de \
--cc=johannes@sipsolutions$(echo .)net \
--cc=linux-doc@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nicolas.dichtel@6wind$(echo .)com \
--cc=pabeni@redhat$(echo .)com \
--cc=razor@blackwall$(echo .)org \
--cc=robh@kernel$(echo .)org \
--cc=sdf@google$(echo .)com \
--cc=stephen@networkplumber$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox