[RFC PATCH v2 net-next 0/3] netlink: formatted extacks

[RFC PATCH v2 net-next 0/3] netlink: formatted extacks

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

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 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                   | 25 +++++++++++-
 8 files changed, 46 insertions(+), 92 deletions(-)

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

[edward.cree]

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 | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index d51e041d2242..4cbe87739c4d 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,27 @@ 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);			\
+									\
+	if (!__extack)							\
+		break;							\
+	if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
+		     (fmt), ##args) >= NETLINK_MAX_FMTMSG_LEN)		\
+		net_warn_ratelimited("truncated extack: " fmt "\n",	\
+				     ##args);				\
+									\
+	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);		\

[RFC PATCH v2 net-next 2/3] sfc: use formatted extacks instead of efx_tc_err()

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

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.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 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(-)

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 874c765b2465..6f472ea0638a 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -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;
diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index 3478860d4023..b21a961eabb1 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -137,17 +137,16 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 		flow_rule_match_control(rule, &fm);
 
 		if (fm.mask->flags) {
-			efx_tc_err(efx, "Unsupported match on control.flags %#x\n",
-				   fm.mask->flags);
-			NL_SET_ERR_MSG_MOD(extack, "Unsupported match on control.flags");
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported match on control.flags %#x",
+					       fm.mask->flags);
 			return -EOPNOTSUPP;
 		}
 	}
 	if (dissector->used_keys &
 	    ~(BIT(FLOW_DISSECTOR_KEY_CONTROL) |
 	      BIT(FLOW_DISSECTOR_KEY_BASIC))) {
-		efx_tc_err(efx, "Unsupported flower keys %#x\n", dissector->used_keys);
-		NL_SET_ERR_MSG_MOD(extack, "Unsupported flower keys encountered");
+		NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported flower keys %#x",
+				       dissector->used_keys);
 		return -EOPNOTSUPP;
 	}
 
@@ -156,11 +155,11 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 
 		flow_rule_match_basic(rule, &fm);
 		if (fm.mask->n_proto) {
-			EFX_TC_ERR_MSG(efx, extack, "Unsupported eth_proto match\n");
+			NL_SET_ERR_MSG_MOD(extack, "Unsupported eth_proto match");
 			return -EOPNOTSUPP;
 		}
 		if (fm.mask->ip_proto) {
-			EFX_TC_ERR_MSG(efx, extack, "Unsupported ip_proto match\n");
+			NL_SET_ERR_MSG_MOD(extack, "Unsupported ip_proto match");
 			return -EOPNOTSUPP;
 		}
 	}
@@ -200,13 +199,9 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 	if (efv != from_efv) {
 		/* can't happen */
-		efx_tc_err(efx, "for %s efv is %snull but from_efv is %snull\n",
-			   netdev_name(net_dev), efv ? "non-" : "",
-			   from_efv ? "non-" : "");
-		if (efv)
-			NL_SET_ERR_MSG_MOD(extack, "vfrep filter has PF net_dev (can't happen)");
-		else
-			NL_SET_ERR_MSG_MOD(extack, "PF filter has vfrep net_dev (can't happen)");
+		NL_SET_ERR_MSG_FMT_MOD(extack, "for %s efv is %snull but from_efv is %snull (can't happen)",
+				       netdev_name(net_dev), efv ? "non-" : "",
+				       from_efv ? "non-" : "");
 		return -EINVAL;
 	}
 
@@ -214,7 +209,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 	memset(&match, 0, sizeof(match));
 	rc = efx_tc_flower_external_mport(efx, from_efv);
 	if (rc < 0) {
-		EFX_TC_ERR_MSG(efx, extack, "Failed to identify ingress m-port");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to identify ingress m-port");
 		return rc;
 	}
 	match.value.ingress_port = rc;
@@ -224,7 +219,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 		return rc;
 
 	if (tc->common.chain_index) {
-		EFX_TC_ERR_MSG(efx, extack, "No support for nonzero chain_index");
+		NL_SET_ERR_MSG_MOD(extack, "No support for nonzero chain_index");
 		return -EOPNOTSUPP;
 	}
 	match.mask.recirc_id = 0xff;
@@ -261,7 +256,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 		if (!act) {
 			/* more actions after a non-pipe action */
-			EFX_TC_ERR_MSG(efx, extack, "Action follows non-pipe action");
+			NL_SET_ERR_MSG_MOD(extack, "Action follows non-pipe action");
 			rc = -EINVAL;
 			goto release;
 		}
@@ -270,7 +265,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 		case FLOW_ACTION_DROP:
 			rc = efx_mae_alloc_action_set(efx, act);
 			if (rc) {
-				EFX_TC_ERR_MSG(efx, extack, "Failed to write action set to hw (drop)");
+				NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (drop)");
 				goto release;
 			}
 			list_add_tail(&act->list, &rule->acts.list);
@@ -281,20 +276,20 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 			save = *act;
 			to_efv = efx_tc_flower_lookup_efv(efx, fa->dev);
 			if (IS_ERR(to_efv)) {
-				EFX_TC_ERR_MSG(efx, extack, "Mirred egress device not on switch");
+				NL_SET_ERR_MSG_MOD(extack, "Mirred egress device not on switch");
 				rc = PTR_ERR(to_efv);
 				goto release;
 			}
 			rc = efx_tc_flower_external_mport(efx, to_efv);
 			if (rc < 0) {
-				EFX_TC_ERR_MSG(efx, extack, "Failed to identify egress m-port");
+				NL_SET_ERR_MSG_MOD(extack, "Failed to identify egress m-port");
 				goto release;
 			}
 			act->dest_mport = rc;
 			act->deliver = 1;
 			rc = efx_mae_alloc_action_set(efx, act);
 			if (rc) {
-				EFX_TC_ERR_MSG(efx, extack, "Failed to write action set to hw (mirred)");
+				NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (mirred)");
 				goto release;
 			}
 			list_add_tail(&act->list, &rule->acts.list);
@@ -310,9 +305,9 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 			*act = save;
 			break;
 		default:
-			efx_tc_err(efx, "Unhandled action %u\n", fa->id);
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
+					       fa->id);
 			rc = -EOPNOTSUPP;
-			NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
 			goto release;
 		}
 	}
@@ -334,7 +329,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 		act->deliver = 1;
 		rc = efx_mae_alloc_action_set(efx, act);
 		if (rc) {
-			EFX_TC_ERR_MSG(efx, extack, "Failed to write action set to hw (deliver)");
+			NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (deliver)");
 			goto release;
 		}
 		list_add_tail(&act->list, &rule->acts.list);
@@ -349,13 +344,13 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 	rc = efx_mae_alloc_action_set_list(efx, &rule->acts);
 	if (rc) {
-		EFX_TC_ERR_MSG(efx, extack, "Failed to write action set list to hw");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to write action set list to hw");
 		goto release;
 	}
 	rc = efx_mae_insert_rule(efx, &rule->match, EFX_TC_PRIO_TC,
 				 rule->acts.fw_id, &rule->fw_id);
 	if (rc) {
-		EFX_TC_ERR_MSG(efx, extack, "Failed to insert rule in hw");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to insert rule in hw");
 		goto release_acts;
 	}
 	return 0;
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index 196fd74ed973..4373c3243e3c 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -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;

[RFC PATCH v2 net-next 3/3] sfc: remove 'log-tc-errors' ethtool private flag

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

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.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 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(-)

diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 135ece2f1375..702abbe59b76 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -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,
diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c
index 6649a2327d03..a8cbceeb301b 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.c
+++ b/drivers/net/ethernet/sfc/ethtool_common.c
@@ -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)
diff --git a/drivers/net/ethernet/sfc/ethtool_common.h b/drivers/net/ethernet/sfc/ethtool_common.h
index 0afc74021a5e..659491932101 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.h
+++ b/drivers/net/ethernet/sfc/ethtool_common.h
@@ -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);
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 2e9ba0cfe848..7ef823d7a89a 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -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;

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

[Jakub Kicinski]

On Thu, 13 Oct 2022 10:23:00 +0100 edward.cree@amd.com wrote:
> +	if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
> +		     (fmt), ##args) >= NETLINK_MAX_FMTMSG_LEN)		\
> +		net_warn_ratelimited("truncated extack: " fmt "\n",	\
> +				     ##args);				\
> +									\

Some "take it or leave it" comments:
 - Jiri's idea of always printing may be worth exploring
 - my preference would also be to produce a warning on overflow,
   rather than the exact print, because I always worry about people
   starting to depend on the print.

   And WARN_ON() is really heavy and may trigger remediations even
   tho truncated extack is just a minor nuisance.

   I'd do:

   pr(extack formatting overflow $__FILE__:$__func__:$__LINE__ $needed_len)
   
   (I think splicing the "trunced extack:" with fmt will result
    in the format string getting stored in .ro twice?)

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

[Johannes Berg]

On Thu, 2022-10-13 at 08:29 -0700, Jakub Kicinski wrote:
> 
>    I'd do:
> 
>    pr(extack formatting overflow $__FILE__:$__func__:$__LINE__ $needed_len)
>    
>    (I think splicing the "trunced extack:" with fmt will result
>     in the format string getting stored in .ro twice?)
> 

If you worry about the strings (and sizes) then you probably shouldn't
advocate always having __FILE__ and __func__ ;-)

FWIW, my argument earlier was that if we have the truncated string

 a) it lets you recover better in a live system
 b) the message ought to be enough to figure out where the issue is, and
    if the message isn't unique you probably have the problem twice too

But yeah, I'm with "take it or leave it", it all doesn't matter that
much.

johannes

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

[Jakub Kicinski]

On Thu, 13 Oct 2022 18:16:47 +0200 Johannes Berg wrote:
> If you worry about the strings (and sizes) then you probably shouldn't
> advocate always having __FILE__ and __func__ ;-)

To be clear I meant to pass those as arguments to a common format
string. Are you saying just using them will bloat the binary?
Oh well, I guess :(

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

[Edward Cree]

On 13/10/2022 16:29, Jakub Kicinski wrote:
>    (I think splicing the "trunced extack:" with fmt will result
>     in the format string getting stored in .ro twice?)

Yes, it will.  I guess we could splice "%s" with fmt in _both_
 calls (snprintf and net_warn_ratelimited), pass "" to one and
 "truncated extack: " to the other.  Then there's only a single
 string to put in .ro.  Is that worth the complication?

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

[Jakub Kicinski]

On Mon, 17 Oct 2022 13:04:35 +0100 Edward Cree wrote:
> On 13/10/2022 16:29, Jakub Kicinski wrote:
> >    (I think splicing the "trunced extack:" with fmt will result
> >     in the format string getting stored in .ro twice?)  
> 
> Yes, it will.  I guess we could splice "%s" with fmt in _both_
>  calls (snprintf and net_warn_ratelimited), pass "" to one and
>  "truncated extack: " to the other.  Then there's only a single
>  string to put in .ro.  Is that worth the complication?

I vote 'yes', with a simple comment next to it, it should be a fairly
obvious trick to a reader of this code.