Re: [PATCH net-next 2/2] bonding: combine netlink and console error messages

On Sun, Aug 08, 2021 at 09:42:46PM -0400, Jonathan Toppins wrote:

On 8/8/21 6:16 AM, Leon Romanovsky wrote:

quoted

On Fri, Aug 06, 2021 at 11:30:55PM -0400, Jonathan Toppins wrote:

quoted

There seems to be no reason to have different error messages between
netlink and printk. It also cleans up the function slightly.

Signed-off-by: Jonathan Toppins <redacted>
---
  drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++---------------
  1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3ba5f4871162..46b95175690b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1712,6 +1712,16 @@ void bond_lower_state_changed(struct slave *slave)
  	netdev_lower_state_changed(slave->dev, &info);
  }
+#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
+	NL_SET_ERR_MSG(extack, errmsg);				\
+	netdev_err(bond_dev, "Error: " errmsg "\n");		\
+} while (0)
+
+#define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg) do {	\
+	NL_SET_ERR_MSG(extack, errmsg);				\
+	slave_err(bond_dev, slave_dev, "Error: " errmsg "\n");	\
+} while (0)

I don't think that both extack messages and dmesg prints are needed.

They both will be caused by the same source, and both will be seen by
the caller, but duplicated.

IMHO, errors that came from the netlink, should be printed with NL_SET_ERR_MSG(),
other errors should use netdev_err/slave_err prints.

bond_enslave can be called from two places sysfs and netlink so reporting
both a console message and netlink message makes sense to me. So I have to
disagree in this case. I am simply making the two paths report the same
error in the function so that when using sysfs the same information is
reported. In the netlink case the information will be reported twice, once
an an error response over netlink and once via printk.

There is no need to print any errors twice, just add "if (extack)" to you
macros, something like that:

+#define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg) do { 
*       if (extack) 						\
+       	NL_SET_ERR_MSG(extack, errmsg);                 \
+       else 							\
+       	slave_err(bond_dev, slave_dev, "Error: " errmsg "\n");  \
+} while (0)

Thanks

-Jon