netlink: formatted extacks
Subsystems (3)
v3 cover netlink: formatted extacks
From: Edward Cree <[email protected]>
Currently, netlink extacks can only carry fixed string messages, which
is limiting when reporting failures in complex systems. This series adds the ability to return printf-formatted messages, and uses it in the sfc driver's TC offload code.
Formatted extack messages are limited in length to a fixed buffer size,
currently 80 characters. If the message exceeds this, the full message will be logged (ratelimited) to the console and a truncated version returned over netlink.
There is no change to the netlink uAPI; only internal kernel changes
are needed.
Changed in v3:
* altered string splicing in NL_SET_ERR_MSG_FMT to avoid storing the format string twice in .rodata
* removed RFC tags
Changed in v2:
* fixed null-checking of extack (with break; as suggested by kuba)
* added logging of full string on truncation (Johannes)
Edward Cree (3):
netlink: add support for formatted extack messages
sfc: use formatted extacks instead of efx_tc_err()
sfc: remove 'log-tc-errors' ethtool private flag
drivers/net/ethernet/sfc/ef100_ethtool.c | 2 - drivers/net/ethernet/sfc/ethtool_common.c | 37 ------------------ drivers/net/ethernet/sfc/ethtool_common.h | 2 - drivers/net/ethernet/sfc/mae.c | 5 +-- drivers/net/ethernet/sfc/net_driver.h | 2 - drivers/net/ethernet/sfc/tc.c | 47 ++++++++++------------- drivers/net/ethernet/sfc/tc.h | 18 --------- include/linux/netlink.h | 29 +++++++++++++- 8 files changed, 50 insertions(+), 92 deletions(-)
PATCH v3 1/3 netlink: add support for formatted extack messages
From: Edward Cree <[email protected]>
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.
--- include/linux/netlink.h | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)
include/linux/netlink.h +27 -2
@@ -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,31 @@ struct netlink_ext_ack { __extack->_msg = __msg; \ } while (0) +/* We splice fmt with %s at each end even in the snprintf so that both calls + * can use the same string constant, avoiding its duplication in .ro + */ +#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do { \ + struct netlink_ext_ack *__extack = (extack); \ + \ + if (!__extack) \ + break; \ + if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN, \ + "%s" fmt "%s", "", ##args, "") >= \ + NETLINK_MAX_FMTMSG_LEN) \ + net_warn_ratelimited("%s" fmt "%s", "truncated extack: ", \ + ##args, "\n"); \ + \ + do_trace_netlink_extack(__extack->_msg_buf); \ + \ + __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...) \ + 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); \
Re: [PATCH v3 net-next 1/3] netlink: add support for formatted extack messages
Quoted 8 lines · On Tue, 18 Oct 2022 15:37:27 +0100 [email protected] wrote:
From: Edward Cree <[email protected]>
Include an 80-byte buffer in struct
netlink_ext_ackthat can be used
for scnprintf()ed messages. This does mean that the resulting string can't be enumerated, translated etc. in the wayNL_SET_ERR_MSG()was designed to allow.Signed-off-by Edward Cree <[email protected]>
Thanks!
Jakub Kicinski
maintainerRe: [PATCH v3 net-next 1/3] netlink: add support for formatted extack messages
In reply to [email protected]Quoted 4 lines · On Tue, 18 Oct 2022 15:37:27 +0100 [email protected] wrote:
Include an 80-byte buffer in struct
netlink_ext_ackthat can be used
for scnprintf()ed messages. This does mean that the resulting string can't be enumerated, translated etc. in the wayNL_SET_ERR_MSG()was designed to allow.
I only read the code not the message yesterday, but FWIW I think the translation problem is surmountable within the format string itself. But let's worry about that if someone actually comes forward and wants to work on the translations :/
PATCH v3 2/3 sfc: use formatted extacks instead of efx_tc_err()
In reply to [email protected]From: Edward Cree <[email protected]>
Since we can now get a formatted message back to the user with
NL_SET_ERR_MSG_FMT_MOD(), there's no need for our special logging.
--- drivers/net/ethernet/sfc/mae.c | 5 ++-- drivers/net/ethernet/sfc/tc.c | 47 +++++++++++++++------------------- drivers/net/ethernet/sfc/tc.h | 18 ------------- 3 files changed, 23 insertions(+), 47 deletions(-)
drivers/net/ethernet/sfc/mae.c +2 -3
@@ -265,9 +265,8 @@ int efx_mae_match_check_caps(struct efx_nic *efx, rc =efx_mae_match_check_cap_typ (supported_fields[MAE_FIELD_INGRESS_PORT ], ingress_port_mask_type);if (rc) { -efx_tc_err (efx,"No support for %s mask in field ingress_port\n" , -mask_type_name (ingress_port_mask_type)); -NL_SET_ERR_MSG_MOD (extack,"Unsupported mask type for ingress_port" ); +NL_SET_ERR_MSG_FMT_MOD (extack,"No support for %s mask in field ingress_port" , +mask_type_name (ingress_port_mask_type));return rc; }return 0 ;
drivers/net/ethernet/sfc/tc.c +21 -26
139 lines not on this page · raw message
drivers/net/ethernet/sfc/tc.h +0 -18
@@ -15,24 +15,6 @@#include <linux/rhashtable.h> #include "net_driver.h" -/* Error reporting: convenience macros. For indicating why a given filter -* insertion is not supported; errors in internal operation or in the -* hardware should be netif_err()s instead. -*/ -/* Used when error message is constant. */ -#define EFX_TC_ERR_MSG (efx, extack, message) do { \ - NL_SET_ERR_MSG_MOD(extack, message); \ - if (efx->log_tc_errs) \ - netif_info(efx, drv, efx->net_dev, "%s\n", message); \ -} while (0) -/* Used when error message is not constant; caller should also supply a -* constant extack message with NL_SET_ERR_MSG_MOD(). -*/ -#define efx_tc_err(efx, fmt, args...) do { \ -if (efx->log_tc_errs) \ - netif_info(efx, drv, efx->net_dev, fmt, ##args);\ -} while (0) -struct efx_tc_action_set {u16 deliver:1 ;u32 dest_mport;
PATCH v3 3/3 sfc: remove 'log-tc-errors' ethtool private flag
In reply to [email protected]From: Edward Cree <[email protected]>
It no longer does anything now that we're using formatted extacks instead. So we can remove the driver's whole get/set priv_flags implementation.
--- drivers/net/ethernet/sfc/ef100_ethtool.c | 2 -- drivers/net/ethernet/sfc/ethtool_common.c | 37 ----------------------- drivers/net/ethernet/sfc/ethtool_common.h | 2 -- drivers/net/ethernet/sfc/net_driver.h | 2 -- 4 files changed, 43 deletions(-)
drivers/net/ethernet/sfc/ef100_ethtool.c +0 -2
@@ -43,8 +43,6 @@ const struct ethtool_ops ef100_ethtool_ops = { .get_pauseparam = efx_ethtool_get_pauseparam, .set_pauseparam = efx_ethtool_set_pauseparam, .get_sset_count = efx_ethtool_get_sset_count, - .get_priv_flags = efx_ethtool_get_priv_flags, - .set_priv_flags = efx_ethtool_set_priv_flags, .self_test = efx_ethtool_self_test, .get_strings = efx_ethtool_get_strings, .get_link_ksettings = efx_ethtool_get_link_ksettings,
drivers/net/ethernet/sfc/ethtool_common.c +0 -37
@@ -101,14 +101,6 @@ static const struct efx_sw_stat_desc efx_sw_stat_desc[] = {#define EFX_ETHTOOL_SW_STAT_COUNT ARRAY_SIZE(efx_sw_stat_desc) -static const char efx_ethtool_priv_flags_strings[][ETH_GSTRING_LEN ] = { -"log-tc-errors" , -}; - -#define EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS BIT(0) - -#define EFX_ETHTOOL_PRIV_FLAGS_COUNT ARRAY_SIZE(efx_ethtool_priv_flags_strings) -void efx_ethtool_get_drvinfo (struct net_device *net_dev,struct ethtool_drvinfo *info) {
@@ -460,8 +452,6 @@ int efx_ethtool_get_sset_count(struct net_device *net_dev, int string_set)efx_ptp_describe_stats (efx,NULL );case ETH_SS_TEST :return efx_ethtool_fill_self_tests (efx,NULL ,NULL ,NULL ); -case ETH_SS_PRIV_FLAGS : -return EFX_ETHTOOL_PRIV_FLAGS_COUNT ;default :return -EINVAL ; }
@@ -488,39 +478,12 @@ void efx_ethtool_get_strings(struct net_device *net_dev,case ETH_SS_TEST :efx_ethtool_fill_self_tests (efx,NULL , strings,NULL );break ; -case ETH_SS_PRIV_FLAGS : -for (i =0 ; i <EFX_ETHTOOL_PRIV_FLAGS_COUNT ; i++) -strscpy (strings + i *ETH_GSTRING_LEN , - efx_ethtool_priv_flags_strings[i], -ETH_GSTRING_LEN ); -break ;default :/* No other string sets */ break ; } } -u32 efx_ethtool_get_priv_flags (struct net_device *net_dev) -{ -struct efx_nic *efx =efx_netdev_priv (net_dev); -u32 ret_flags =0 ; - -if (efx->log_tc_errs) - ret_flags |=EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS ; - -return ret_flags; -} - -int efx_ethtool_set_priv_flags (struct net_device *net_dev,u32 flags) -{ -struct efx_nic *efx =efx_netdev_priv (net_dev); - - efx->log_tc_errs = - !!(flags &EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS ); - -return 0 ; -} -void efx_ethtool_get_stats (struct net_device *net_dev,struct ethtool_stats *stats,u64 *data)
drivers/net/ethernet/sfc/ethtool_common.h +0 -2
@@ -27,8 +27,6 @@ int efx_ethtool_fill_self_tests(struct efx_nic *efx,int efx_ethtool_get_sset_count (struct net_device *net_dev,int string_set);void efx_ethtool_get_strings (struct net_device *net_dev,u32 string_set,u8 *strings); -u32 efx_ethtool_get_priv_flags (struct net_device *net_dev); -int efx_ethtool_set_priv_flags (struct net_device *net_dev,u32 flags);void efx_ethtool_get_stats (struct net_device *net_dev,struct ethtool_stats *stats __attribute__ ((unused)),u64 *data);
drivers/net/ethernet/sfc/net_driver.h +0 -2
@@ -855,7 +855,6 @@ enum efx_xdp_tx_queues_mode {* @timer_max_ns: Interrupt timer maximum value, in nanoseconds * @irq_rx_adaptive: Adaptive IRQ moderation enabled for RX event queues * @irqs_hooked: Channel interrupts are hooked -* @log_tc_errs: Error logging for TC filter insertion is enabled * @irq_rx_mod_step_us: Step size for IRQ moderation for RX event queues * @irq_rx_moderation_us: IRQ moderation time for RX event queues * @msg_enable: Log message enable flags
@@ -1018,7 +1017,6 @@ struct efx_nic {unsigned int timer_max_ns;bool irq_rx_adaptive;bool irqs_hooked; -bool log_tc_errs;unsigned int irq_mod_step_us;unsigned int irq_rx_moderation_us;u32 msg_enable;
This series was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:
Quoted 13 lines · On Tue, 18 Oct 2022 15:37:26 +0100 you wrote:
From: Edward Cree <[email protected]>
Currently, netlink extacks can only carry fixed string messages, which
is limiting when reporting failures in complex systems. This series adds the ability to return printf-formatted messages, and uses it in the sfc driver's TC offload code.
Formatted extack messages are limited in length to a fixed buffer size,
currently 80 characters. If the message exceeds this, the full message will be logged (ratelimited) to the console and a truncated version returned over netlink.
There is no change to the netlink uAPI; only internal kernel changes
are needed.
Quoted 1 line
[...]
Here is the summary with links:
- [v3,net-next,1/3] netlink: add support for formatted extack messages
https://git.kernel.org/netdev/net-next/c/51c352bdbcd2 - [v3,net-next,2/3] sfc: use formatted extacks instead of efx_tc_err() https://git.kernel.org/netdev/net-next/c/ad1c80d5f777 - [v3,net-next,3/3] sfc: remove 'log-tc-errors' ethtool private flag https://git.kernel.org/netdev/net-next/c/b799f052a987
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html