[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191123165655.5a9b8877@cakuba.netronome.com>
Date: Sat, 23 Nov 2019 16:56:55 -0800
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: Saeed Mahameed <saeedm@...lanox.com>
Cc: "David S. Miller" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Jiri Pirko <jiri@...lanox.com>,
David Ahern <dsahern@...il.com>,
Johannes Berg <johannes@...solutions.net>
Subject: Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a
formattable buffer
On Fri, 22 Nov 2019 22:41:47 +0000, Saeed Mahameed wrote:
> Before this patch the extack msg had to be a const char and didn't leave
> much room for developers to report formattable and flexible messages.
>
> All current usages are oneliner messages, hence, a buffer of size 128B
> should be sufficient to replace the const char pointer.
>
> This will allow future usages to provide formattable messages and more
> flexible error reporting, without any impact on current users.
>
> Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
Let's CC DSA and Johannes on this one
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 205fa7b1f07a..de9004da0288 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -62,6 +62,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 NL_EXTACK_MAX_MSG_SZ 128
>
> /**
> * struct netlink_ext_ack - netlink extended ACK report struct
> @@ -72,40 +73,36 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
> * @cookie_len: actual cookie data length
> */
> struct netlink_ext_ack {
> - const char *_msg;
> + char _msg[NL_EXTACK_MAX_MSG_SZ];
> const struct nlattr *bad_attr;
> u8 cookie[NETLINK_MAX_COOKIE_LEN];
> u8 cookie_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.)
> - */
> -#define NL_SET_ERR_MSG(extack, msg) do { \
> - static const char __msg[] = msg; \
> +#define NL_MSG_FMT(extack, fmt, ...) \
> + WARN_ON(snprintf(extack->_msg, NL_EXTACK_MAX_MSG_SZ, fmt, ## __VA_ARGS__) \
> + >= NL_EXTACK_MAX_MSG_SZ)
I'd personally appreciate a word of analysis and reassurance in the
commit message that this snprintf + WARN_ON inlined in every location
where extack is used won't bloat the kernel :S
One could easily imagine a solution which would continue to carry the
static strings via a pointer without the unnecessary roundtrip thru
snprintf().
> +#define NL_SET_ERR_MSG(extack, fmt, ...) do { \
> struct netlink_ext_ack *__extack = (extack); \
> \
> if (__extack) \
> - __extack->_msg = __msg; \
> + NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
> } while (0)
>
> -#define NL_SET_ERR_MSG_MOD(extack, msg) \
> - NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> +#define NL_SET_ERR_MSG_MOD(extack, fmt, ...) \
> + NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " fmt, ## __VA_ARGS__)
>
> #define NL_SET_BAD_ATTR(extack, attr) do { \
> if ((extack)) \
> (extack)->bad_attr = (attr); \
> } while (0)
>
> -#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do { \
> - static const char __msg[] = msg; \
> +#define NL_SET_ERR_MSG_ATTR(extack, attr, fmt, ...) do { \
> struct netlink_ext_ack *__extack = (extack); \
> \
> if (__extack) { \
> - __extack->_msg = __msg; \
> + NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
> __extack->bad_attr = (attr); \
> } \
> } while (0)
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index cace9b307781..2ce1d6b68ce8 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -208,7 +208,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> case NLA_REJECT:
> if (extack && pt->validation_data) {
> NL_SET_BAD_ATTR(extack, nla);
> - extack->_msg = pt->validation_data;
> + NL_MSG_FMT(extack, pt->validation_data);
> return -EINVAL;
> }
> err = -EINVAL;
Powered by blists - more mailing lists