Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Jiri Pirko <jiri@resnulli.us>]

Fri, Oct 07, 2022 at 03:25:12PM CEST, ecree@xilinx.com wrote:
>From: Edward Cree <ecree.xilinx@gmail.com>
>
>Include an 80-byte buffer in struct netlink_ext_ack that can be used
> for scnprintf()ed messages.  This does mean that the resulting string
> can't be enumerated, translated etc. in the way NL_SET_ERR_MSG() was
> designed to allow.
>
>Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
>---
> include/linux/netlink.h | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>index d51e041d2242..bfab9dbd64fa 100644
>--- a/include/linux/netlink.h
>+++ b/include/linux/netlink.h
>@@ -64,6 +64,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
> 
> /* this can be increased when necessary - don't expose to userland */
> #define NETLINK_MAX_COOKIE_LEN	20
>+#define NETLINK_MAX_FMTMSG_LEN	80
> 
> /**
>  * struct netlink_ext_ack - netlink extended ACK report struct
>@@ -75,6 +76,8 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>  * @miss_nest: nest missing an attribute (%NULL if missing top level attr)
>  * @cookie: cookie data to return to userspace (for success)
>  * @cookie_len: actual cookie data length
>+ * @_msg_buf: output buffer for formatted message strings - don't access
>+ *	directly, use %NL_SET_ERR_MSG_FMT
>  */
> struct netlink_ext_ack {
> 	const char *_msg;
>@@ -84,13 +87,13 @@ struct netlink_ext_ack {
> 	u16 miss_type;
> 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
> 	u8 cookie_len;
>+	char _msg_buf[NETLINK_MAX_FMTMSG_LEN];
> };
> 
> /* Always use this macro, this allows later putting the
>  * message into a separate section or such for things
>  * like translation or listing all possible messages.
>- * Currently string formatting is not supported (due
>- * to the lack of an output buffer.)
>+ * If string formatting is needed use NL_SET_ERR_MSG_FMT.
>  */
> #define NL_SET_ERR_MSG(extack, msg) do {		\
> 	static const char __msg[] = msg;		\
>@@ -102,9 +105,23 @@ struct netlink_ext_ack {
> 		__extack->_msg = __msg;			\
> } while (0)
> 
>+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
>+	struct netlink_ext_ack *__extack = (extack);		\
>+								\
>+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
>+		  (fmt), ##args);				\
>+	do_trace_netlink_extack(__extack->_msg_buf);		\
>+								\
>+	if (__extack)						\
>+		__extack->_msg = __extack->_msg_buf;		\
>+} while (0)
>+
> #define NL_SET_ERR_MSG_MOD(extack, msg)			\
> 	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> 
>+#define NL_SET_ERR_MSG_FMT_MOD(extack, fmt, args...)	\

I wonder, wouldn't it be better to just have NL_SET_ERR_MSG_MOD which
accepts format string and that's it. I understand there is an extra
overhead for the messages that don't use formatting, but do we care?
This is no fastpath and usually happens only seldom. The API towards
the driver would be more simple.

I like this a lot!


>+	NL_SET_ERR_MSG_FMT((extack), KBUILD_MODNAME ": " fmt, ##args)
>+
> #define NL_SET_BAD_ATTR_POLICY(extack, attr, pol) do {	\
> 	if ((extack)) {					\
> 		(extack)->bad_attr = (attr);		\