[PATCH net-next 0/2] bonding: cleanup header file and error msgs

Two small patches removing unreferenced symbols and unifying error
messages across netlink and printk.

Signed-off-by: Jonathan Toppins <redacted>

Jonathan Toppins (2):
  bonding: remove extraneous definitions from bonding.h
  bonding: combine netlink and console error messages

 drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++---------------
 include/net/bonding.h           | 12 ---------
 2 files changed, 25 insertions(+), 32 deletions(-)

-- 
2.27.0

All of the symbols either only exist in bond_options.c or nowhere at
all. These symbols were verified to not exist in the code base by
using `git grep` and their removal was verified by compiling bonding.ko.

Signed-off-by: Jonathan Toppins <redacted>
---
 include/net/bonding.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/include/net/bonding.h b/include/net/bonding.h
index 46df47004803..2ff4ac65bbe3 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h

@@ -150,11 +150,6 @@ struct bond_params {
 	u8 ad_actor_system[ETH_ALEN + 2];
 };
 
-struct bond_parm_tbl {
-	char *modename;
-	int mode;
-};
-
 struct slave {
 	struct net_device *dev; /* first - useful for panic debug */
 	struct bonding *bond; /* our master */

@@ -754,13 +749,6 @@ static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
 
 /* exported from bond_main.c */
 extern unsigned int bond_net_id;
-extern const struct bond_parm_tbl bond_lacp_tbl[];
-extern const struct bond_parm_tbl xmit_hashtype_tbl[];
-extern const struct bond_parm_tbl arp_validate_tbl[];
-extern const struct bond_parm_tbl arp_all_targets_tbl[];
-extern const struct bond_parm_tbl fail_over_mac_tbl[];
-extern const struct bond_parm_tbl pri_reselect_tbl[];
-extern struct bond_parm_tbl ad_select_tbl[];
 
 /* exported from bond_netlink.c */
 extern struct rtnl_link_ops bond_link_ops;

-- 
2.27.0

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)
+
 /* enslave device <slave> to bond device <master> */
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		 struct netlink_ext_ack *extack)

@@ -1725,9 +1735,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	if (slave_dev->flags & IFF_MASTER &&
 	    !netif_is_bond_master(slave_dev)) {
-		NL_SET_ERR_MSG(extack, "Device with IFF_MASTER cannot be enslaved");
-		netdev_err(bond_dev,
-			   "Error: Device with IFF_MASTER cannot be enslaved\n");
+		BOND_NL_ERR(bond_dev, extack,
+			    "Device with IFF_MASTER cannot be enslaved");
 		return -EPERM;
 	}
 

@@ -1739,15 +1748,13 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	/* already in-use? */
 	if (netdev_is_rx_handler_busy(slave_dev)) {
-		NL_SET_ERR_MSG(extack, "Device is in use and cannot be enslaved");
-		slave_err(bond_dev, slave_dev,
-			  "Error: Device is in use and cannot be enslaved\n");
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device is in use and cannot be enslaved");
 		return -EBUSY;
 	}
 
 	if (bond_dev == slave_dev) {
-		NL_SET_ERR_MSG(extack, "Cannot enslave bond to itself.");
-		netdev_err(bond_dev, "cannot enslave bond to itself.\n");
+		BOND_NL_ERR(bond_dev, extack, "Cannot enslave bond to itself.");
 		return -EPERM;
 	}
 

@@ -1756,8 +1763,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) {
 		slave_dbg(bond_dev, slave_dev, "is NETIF_F_VLAN_CHALLENGED\n");
 		if (vlan_uses_dev(bond_dev)) {
-			NL_SET_ERR_MSG(extack, "Can not enslave VLAN challenged device to VLAN enabled bond");
-			slave_err(bond_dev, slave_dev, "Error: cannot enslave VLAN challenged slave on VLAN enabled bond\n");
+			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+				     "Can not enslave VLAN challenged device to VLAN enabled bond");
 			return -EPERM;
 		} else {
 			slave_warn(bond_dev, slave_dev, "enslaved VLAN challenged slave. Adding VLANs will be blocked as long as it is part of bond.\n");

@@ -1775,8 +1782,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	 * enslaving it; the old ifenslave will not.
 	 */
 	if (slave_dev->flags & IFF_UP) {
-		NL_SET_ERR_MSG(extack, "Device can not be enslaved while up");
-		slave_err(bond_dev, slave_dev, "slave is up - this may be due to an out of date ifenslave\n");
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device can not be enslaved while up");
 		return -EPERM;
 	}
 

@@ -1815,17 +1822,15 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 						 bond_dev);
 		}
 	} else if (bond_dev->type != slave_dev->type) {
-		NL_SET_ERR_MSG(extack, "Device type is different from other slaves");
-		slave_err(bond_dev, slave_dev, "ether type (%d) is different from other slaves (%d), can not enslave it\n",
-			  slave_dev->type, bond_dev->type);
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device type is different from other slaves");
 		return -EINVAL;
 	}
 
 	if (slave_dev->type == ARPHRD_INFINIBAND &&
 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
-		NL_SET_ERR_MSG(extack, "Only active-backup mode is supported for infiniband slaves");
-		slave_warn(bond_dev, slave_dev, "Type (%d) supports only active-backup mode\n",
-			   slave_dev->type);
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Only active-backup mode is supported for infiniband slaves");
 		res = -EOPNOTSUPP;
 		goto err_undo_flags;
 	}

@@ -1839,8 +1844,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 				bond->params.fail_over_mac = BOND_FOM_ACTIVE;
 				slave_warn(bond_dev, slave_dev, "Setting fail_over_mac to active for active-backup mode\n");
 			} else {
-				NL_SET_ERR_MSG(extack, "Slave device does not support setting the MAC address, but fail_over_mac is not set to active");
-				slave_err(bond_dev, slave_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n");
+				SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+					     "Slave device does not support setting the MAC address, but fail_over_mac is not set to active");
 				res = -EOPNOTSUPP;
 				goto err_undo_flags;
 			}

-- 
2.27.0

On Fri, 2021-08-06 at 23:30 -0400, Jonathan Toppins wrote:

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

[]

[]

+#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)

If you are doing this, it's probably smaller object code to use
	"%s", errmsg 
as the errmsg string can be reused

#define BOND_NL_ERR(bond_dev, extack, errmsg)			\
do {								\
	NL_SET_ERR_MSG(extack, errmsg);				\
	netdev_err(bond_dev, "Error: %s\n", errmsg);		\
} 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: %s\n", errmsg);	\
} while (0)

On 8/6/21 11:52 PM, Joe Perches wrote:

On Fri, 2021-08-06 at 23:30 -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.

[]

quoted

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c

[]

quoted

+#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)

If you are doing this, it's probably smaller object code to use
	"%s", errmsg
as the errmsg string can be reused

#define BOND_NL_ERR(bond_dev, extack, errmsg)			\
do {								\
	NL_SET_ERR_MSG(extack, errmsg);				\
	netdev_err(bond_dev, "Error: %s\n", errmsg);		\
} 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: %s\n", errmsg);	\
} while (0)

I like the thought and would agree if not for how NL_SET_ERR_MSG is 
coded. Unfortunately it does not appear as though doing the above change 
actually generates smaller object code. Maybe I have incorrectly 
interpreted something?

$ git show
commit 6bb346263b4e9d008744b45af5105df309c35c1a (HEAD -> 
upstream-bonding-cleanup)
Author: Jonathan Toppins [off-list ref]
Date:   Sat Aug 7 17:34:58 2021 -0400

     object code optimization

diff --git a/drivers/net/bonding/bond_main.c 

b/drivers/net/bonding/bond_main.c
index 46b95175690b..e2903ae7cdab 100644

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1714,12 +1714,12 @@ void bond_lower_state_changed(struct slave *slave)

  #define BOND_NL_ERR(bond_dev, extack, errmsg) do {             \
         NL_SET_ERR_MSG(extack, errmsg);                         \
-       netdev_err(bond_dev, "Error: " errmsg "\n");            \
+       netdev_err(bond_dev, "Error: %s\n", errmsg);            \
  } 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");  \
+       slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);  \
  } while (0)

  /* enslave device <slave> to bond device <master> */

$ git log --oneline -3
6bb346263b4e (HEAD -> upstream-bonding-cleanup) object code optimization
a36c7639a139 bonding: combine netlink and console error messages
88916c847e85 bonding: remove extraneous definitions from bonding.h
jtoppins@jtoppins:~/projects/linux-rhel7$ git rebase -i --exec "make 
drivers/net/bonding/bond_main.o; ls -l drivers/net/bonding/bond_main.o" 
HEAD^^
hint: Waiting for your editor to close the file... Error detected while 
processing 
/home/jtoppins/.vim/bundle/cscope_macros.vim/plugin/cscope_macros.vim:
line   42:
E568: duplicate cscope database not added
Press ENTER or type command to continue
Executing: make menuconfig


*** End of the configuration.
*** Execute 'make' to start the build or try 'make help'.

Executing: make drivers/net/bonding/bond_main.o; ls -l 
drivers/net/bonding/bond_main.o
   CALL    scripts/checksyscalls.sh
   CALL    scripts/atomic/check-atomics.sh
   DESCEND objtool
   DESCEND bpf/resolve_btfids
   CC [M]  drivers/net/bonding/bond_main.o
-rw-r--r--. 1 jtoppins jtoppins 1777896 Aug  7 17:37 
drivers/net/bonding/bond_main.o
Executing: make drivers/net/bonding/bond_main.o; ls -l 
drivers/net/bonding/bond_main.o
   CALL    scripts/checksyscalls.sh
   CALL    scripts/atomic/check-atomics.sh
   DESCEND objtool
   DESCEND bpf/resolve_btfids
   CC [M]  drivers/net/bonding/bond_main.o
-rw-r--r--. 1 jtoppins jtoppins 1778320 Aug  7 17:37 
drivers/net/bonding/bond_main.o
Successfully rebased and updated refs/heads/upstream-bonding-cleanup.

It appears the change increases bond_main.o by 424 (1778320-1777896) bytes.

On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:

On 8/6/21 11:52 PM, Joe Perches wrote:

quoted

On Fri, 2021-08-06 at 23:30 -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.

[]

quoted

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c

[]

quoted

+#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)

If you are doing this, it's probably smaller object code to use
	"%s", errmsg
as the errmsg string can be reused

#define BOND_NL_ERR(bond_dev, extack, errmsg)			\
do {								\
	NL_SET_ERR_MSG(extack, errmsg);				\
	netdev_err(bond_dev, "Error: %s\n", errmsg);		\
} 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: %s\n", errmsg);	\
} while (0)

I like the thought and would agree if not for how NL_SET_ERR_MSG is 
coded. Unfortunately it does not appear as though doing the above change 
actually generates smaller object code. Maybe I have incorrectly 
interpreted something?

No, it's because you are compiling allyesconfig or equivalent.
Try defconfig with bonding.

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

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.

Thanks

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

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.

-Jon

On 8/8/21 6:02 AM, Joe Perches wrote:

On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:

quoted

On 8/6/21 11:52 PM, Joe Perches wrote:

quoted

On Fri, 2021-08-06 at 23:30 -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.

[]

quoted

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c

[]

quoted

+#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)

If you are doing this, it's probably smaller object code to use
	"%s", errmsg
as the errmsg string can be reused

#define BOND_NL_ERR(bond_dev, extack, errmsg)			\
do {								\
	NL_SET_ERR_MSG(extack, errmsg);				\
	netdev_err(bond_dev, "Error: %s\n", errmsg);		\
} 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: %s\n", errmsg);	\
} while (0)

I like the thought and would agree if not for how NL_SET_ERR_MSG is
coded. Unfortunately it does not appear as though doing the above change
actually generates smaller object code. Maybe I have incorrectly
interpreted something?

No, it's because you are compiling allyesconfig or equivalent.
Try defconfig with bonding.

$ git clean -dxf
$ git log -1 -p
commit 8985f8d3fa38bca5f5384f9210ed735d58fd94f2 (HEAD -> 
upstream-bonding-cleanup)
Author: Jonathan Toppins [off-list ref]
Date:   Sun Aug 8 21:45:14 2021 -0400

     object code optimization

diff --git a/drivers/net/bonding/bond_main.c 

b/drivers/net/bonding/bond_main.c
index 46b95175690b..e2903ae7cdab 100644

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1714,12 +1714,12 @@ void bond_lower_state_changed(struct slave *slave)

  #define BOND_NL_ERR(bond_dev, extack, errmsg) do {             \
         NL_SET_ERR_MSG(extack, errmsg);                         \
-       netdev_err(bond_dev, "Error: " errmsg "\n");            \
+       netdev_err(bond_dev, "Error: %s\n", errmsg);            \
  } 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");  \
+       slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);  \
  } while (0)

  /* enslave device <slave> to bond device <master> */

$ git log --oneline -2
8985f8d3fa38 (HEAD -> upstream-bonding-cleanup) object code optimization
e326bf8fd30f bonding: combine netlink and console error messages
$ make defconfig
   HOSTCC  scripts/basic/fixdep
[...]
*** Default configuration is based on 'x86_64_defconfig'
#
# configuration written to .config
#
$ grep "BONDING" .config
# CONFIG_BONDING is not set
$ make menuconfig
   UPD     scripts/kconfig/mconf-cfg
[...]
configuration written to .config

*** End of the configuration.
*** Execute 'make' to start the build or try 'make help'.

$ grep "BONDING" .config
CONFIG_BONDING=m
$ git rebase -i --exec "make drivers/net/bonding/bond_main.o; ls -l 
drivers/net/bonding/bond_main.o" HEAD^^
Executing: make drivers/net/bonding/bond_main.o; ls -l 
drivers/net/bonding/bond_main.o
   SYNC    include/config/auto.conf.cmd
[...]
   CC      /home/jtoppins/projects/linux-rhel7/tools/objtool/librbtree.o
   LD      /home/jtoppins/projects/linux-rhel7/tools/objtool/objtool-in.o
   LINK    /home/jtoppins/projects/linux-rhel7/tools/objtool/objtool
   CC [M]  drivers/net/bonding/bond_main.o
-rw-r--r--. 1 jtoppins jtoppins 131800 Aug  8 21:47 
drivers/net/bonding/bond_main.o
Executing: make drivers/net/bonding/bond_main.o; ls -l 
drivers/net/bonding/bond_main.o
   CALL    scripts/checksyscalls.sh
   CALL    scripts/atomic/check-atomics.sh
   DESCEND objtool
   CC [M]  drivers/net/bonding/bond_main.o
-rw-r--r--. 1 jtoppins jtoppins 131928 Aug  8 21:47 
drivers/net/bonding/bond_main.o
Successfully rebased and updated refs/heads/upstream-bonding-cleanup.
$ gcc --version
gcc (GCC) 8.4.1 20200928 (Red Hat 8.4.1-1)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

-Jon

On Sun, 2021-08-08 at 22:07 -0400, Jonathan Toppins wrote:

Your size is significantly different than mine (x86-64 defconfig w/ bonding)

$ gcc --version
gcc (Ubuntu 10.3.0-1ubuntu1) 10.3.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Original:

$ git log -1
commit 7999516e20bd9bb5d1f7351cbd05ca529a3a8d60 (HEAD, tag: next-20210806, origin/master, origin/HEAD)
Author: Mark Brown [off-list ref]
Date:   Fri Aug 6 17:52:53 2021 +0100

    Add linux-next specific files for 20210806
    
    Signed-off-by: Mark Brown [off-list ref]

$ size drivers/net/bonding/built-in.a -t
   text	   data	    bss	    dec	    hex	filename
  59630	    399	    460	  60489	   ec49	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
  16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
  17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
   7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
   1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
    165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
   6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
  15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
   4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
 129842	   2357	    462	 132661	  20635	(TOTALS)

Then with your 2 patches:

$ size -t drivers/net/bonding/built-in.a
   text	   data	    bss	    dec	    hex	filename
  59590	    399	    460	  60449	   ec21	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
  16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
  17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
   7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
   1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
    165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
   6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
  15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
   4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
 129802	   2357	    462	 132621	  2060d	(TOTALS)

Then with my suggestion:

$ size -t drivers/net/bonding/built-in.a
   text	   data	    bss	    dec	    hex	filename
  59561	    399	    460	  60420	   ec04	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
  16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
  17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
   7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
   1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
    165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
   6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
  15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
   4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
 129773	   2357	    462	 132592	  205f0	(TOTALS)

cheers, Joe

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

On 8/9/21 1:05 AM, Joe Perches wrote:

On Sun, 2021-08-08 at 22:07 -0400, Jonathan Toppins wrote:

quoted

On 8/8/21 6:02 AM, Joe Perches wrote:

quoted

On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:

quoted

On 8/6/21 11:52 PM, Joe Perches wrote:

quoted

On Fri, 2021-08-06 at 23:30 -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.

[]

quoted

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c

[]

quoted

+#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)

If you are doing this, it's probably smaller object code to use
	"%s", errmsg
as the errmsg string can be reused

#define BOND_NL_ERR(bond_dev, extack, errmsg)			\
do {								\
	NL_SET_ERR_MSG(extack, errmsg);				\
	netdev_err(bond_dev, "Error: %s\n", errmsg);		\
} 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: %s\n", errmsg);	\
} while (0)

I like the thought and would agree if not for how NL_SET_ERR_MSG is
coded. Unfortunately it does not appear as though doing the above change
actually generates smaller object code. Maybe I have incorrectly
interpreted something?

No, it's because you are compiling allyesconfig or equivalent.
Try defconfig with bonding.

$ git clean -dxf
$ git log -1 -p
commit 8985f8d3fa38bca5f5384f9210ed735d58fd94f2 (HEAD ->
upstream-bonding-cleanup)
Author: Jonathan Toppins [off-list ref]
Date:   Sun Aug 8 21:45:14 2021 -0400

      object code optimization

diff --git a/drivers/net/bonding/bond_main.c

b/drivers/net/bonding/bond_main.c
index 46b95175690b..e2903ae7cdab 100644

--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1714,12 +1714,12 @@ void bond_lower_state_changed(struct slave *slave)

   #define BOND_NL_ERR(bond_dev, extack, errmsg) do {             \
          NL_SET_ERR_MSG(extack, errmsg);                         \
-       netdev_err(bond_dev, "Error: " errmsg "\n");            \
+       netdev_err(bond_dev, "Error: %s\n", errmsg);            \
   } 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");  \
+       slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);  \
   } while (0)

   /* enslave device <slave> to bond device <master> */

$ git log --oneline -2
8985f8d3fa38 (HEAD -> upstream-bonding-cleanup) object code optimization
e326bf8fd30f bonding: combine netlink and console error messages
$ make defconfig
    HOSTCC  scripts/basic/fixdep
[...]
*** Default configuration is based on 'x86_64_defconfig'
#
# configuration written to .config
#
$ grep "BONDING" .config
# CONFIG_BONDING is not set
$ make menuconfig
    UPD     scripts/kconfig/mconf-cfg
[...]
configuration written to .config

*** End of the configuration.
*** Execute 'make' to start the build or try 'make help'.

$ grep "BONDING" .config
CONFIG_BONDING=m
$ git rebase -i --exec "make drivers/net/bonding/bond_main.o; ls -l
drivers/net/bonding/bond_main.o" HEAD^^
Executing: make drivers/net/bonding/bond_main.o; ls -l
drivers/net/bonding/bond_main.o
    SYNC    include/config/auto.conf.cmd
[...]
    CC      /home/jtoppins/projects/linux-rhel7/tools/objtool/librbtree.o
    LD      /home/jtoppins/projects/linux-rhel7/tools/objtool/objtool-in.o
    LINK    /home/jtoppins/projects/linux-rhel7/tools/objtool/objtool
    CC [M]  drivers/net/bonding/bond_main.o
-rw-r--r--. 1 jtoppins jtoppins 131800 Aug  8 21:47
drivers/net/bonding/bond_main.o
Executing: make drivers/net/bonding/bond_main.o; ls -l
drivers/net/bonding/bond_main.o
    CALL    scripts/checksyscalls.sh
    CALL    scripts/atomic/check-atomics.sh
    DESCEND objtool
    CC [M]  drivers/net/bonding/bond_main.o
-rw-r--r--. 1 jtoppins jtoppins 131928 Aug  8 21:47

Your size is significantly different than mine (x86-64 defconfig w/ bonding)

$ gcc --version
gcc (Ubuntu 10.3.0-1ubuntu1) 10.3.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Original:

$ git log -1
commit 7999516e20bd9bb5d1f7351cbd05ca529a3a8d60 (HEAD, tag: next-20210806, origin/master, origin/HEAD)
Author: Mark Brown [off-list ref]
Date:   Fri Aug 6 17:52:53 2021 +0100

     Add linux-next specific files for 20210806
     
     Signed-off-by: Mark Brown [off-list ref]

$ size drivers/net/bonding/built-in.a -t
    text	   data	    bss	    dec	    hex	filename
   59630	    399	    460	  60489	   ec49	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
   16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
   17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
    7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
    1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
     165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
    6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
   15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
    4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
  129842	   2357	    462	 132661	  20635	(TOTALS)

Then with your 2 patches:

$ size -t drivers/net/bonding/built-in.a
    text	   data	    bss	    dec	    hex	filename
   59590	    399	    460	  60449	   ec21	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
   16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
   17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
    7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
    1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
     165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
    6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
   15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
    4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
  129802	   2357	    462	 132621	  2060d	(TOTALS)

Then with my suggestion:

$ size -t drivers/net/bonding/built-in.a
    text	   data	    bss	    dec	    hex	filename
   59561	    399	    460	  60420	   ec04	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
   16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
   17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
    7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
    1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
     165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
    6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
   15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
    4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
  129773	   2357	    462	 132592	  205f0	(TOTALS)

cheers, Joe

Humm I was just building the .o of the one compilation unit. I wonder if 
there is further optimization later. Will post a v2 with yours and 
Leon's changes later this evening.

Appreciate the suggestions.

-Jon

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

v2:
 - changed the printks to reduce object code slightly
 - emit a single error message based on if netlink or sysfs is
   attempting to enslave

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

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

@@ -1712,6 +1712,20 @@ void bond_lower_state_changed(struct slave *slave)
 	netdev_lower_state_changed(slave->dev, &info);
 }
 
+#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
+	if (extack)						\
+		NL_SET_ERR_MSG(extack, errmsg);			\
+	else							\
+		netdev_err(bond_dev, "Error: %s\n", errmsg);	\
+} while (0)
+
+#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: %s\n", errmsg);	\
+} while (0)
+
 /* enslave device <slave> to bond device <master> */
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		 struct netlink_ext_ack *extack)

@@ -1725,9 +1739,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	if (slave_dev->flags & IFF_MASTER &&
 	    !netif_is_bond_master(slave_dev)) {
-		NL_SET_ERR_MSG(extack, "Device with IFF_MASTER cannot be enslaved");
-		netdev_err(bond_dev,
-			   "Error: Device with IFF_MASTER cannot be enslaved\n");
+		BOND_NL_ERR(bond_dev, extack,
+			    "Device with IFF_MASTER cannot be enslaved");
 		return -EPERM;
 	}
 

@@ -1739,15 +1752,13 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	/* already in-use? */
 	if (netdev_is_rx_handler_busy(slave_dev)) {
-		NL_SET_ERR_MSG(extack, "Device is in use and cannot be enslaved");
-		slave_err(bond_dev, slave_dev,
-			  "Error: Device is in use and cannot be enslaved\n");
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device is in use and cannot be enslaved");
 		return -EBUSY;
 	}
 
 	if (bond_dev == slave_dev) {
-		NL_SET_ERR_MSG(extack, "Cannot enslave bond to itself.");
-		netdev_err(bond_dev, "cannot enslave bond to itself.\n");
+		BOND_NL_ERR(bond_dev, extack, "Cannot enslave bond to itself.");
 		return -EPERM;
 	}
 

@@ -1756,8 +1767,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) {
 		slave_dbg(bond_dev, slave_dev, "is NETIF_F_VLAN_CHALLENGED\n");
 		if (vlan_uses_dev(bond_dev)) {
-			NL_SET_ERR_MSG(extack, "Can not enslave VLAN challenged device to VLAN enabled bond");
-			slave_err(bond_dev, slave_dev, "Error: cannot enslave VLAN challenged slave on VLAN enabled bond\n");
+			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+				     "Can not enslave VLAN challenged device to VLAN enabled bond");
 			return -EPERM;
 		} else {
 			slave_warn(bond_dev, slave_dev, "enslaved VLAN challenged slave. Adding VLANs will be blocked as long as it is part of bond.\n");

@@ -1775,8 +1786,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	 * enslaving it; the old ifenslave will not.
 	 */
 	if (slave_dev->flags & IFF_UP) {
-		NL_SET_ERR_MSG(extack, "Device can not be enslaved while up");
-		slave_err(bond_dev, slave_dev, "slave is up - this may be due to an out of date ifenslave\n");
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device can not be enslaved while up");
 		return -EPERM;
 	}
 

@@ -1815,17 +1826,15 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 						 bond_dev);
 		}
 	} else if (bond_dev->type != slave_dev->type) {
-		NL_SET_ERR_MSG(extack, "Device type is different from other slaves");
-		slave_err(bond_dev, slave_dev, "ether type (%d) is different from other slaves (%d), can not enslave it\n",
-			  slave_dev->type, bond_dev->type);
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device type is different from other slaves");
 		return -EINVAL;
 	}
 
 	if (slave_dev->type == ARPHRD_INFINIBAND &&
 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
-		NL_SET_ERR_MSG(extack, "Only active-backup mode is supported for infiniband slaves");
-		slave_warn(bond_dev, slave_dev, "Type (%d) supports only active-backup mode\n",
-			   slave_dev->type);
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Only active-backup mode is supported for infiniband slaves");
 		res = -EOPNOTSUPP;
 		goto err_undo_flags;
 	}

@@ -1839,8 +1848,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 				bond->params.fail_over_mac = BOND_FOM_ACTIVE;
 				slave_warn(bond_dev, slave_dev, "Setting fail_over_mac to active for active-backup mode\n");
 			} else {
-				NL_SET_ERR_MSG(extack, "Slave device does not support setting the MAC address, but fail_over_mac is not set to active");
-				slave_err(bond_dev, slave_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n");
+				SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+					     "Slave device does not support setting the MAC address, but fail_over_mac is not set to active");
 				res = -EOPNOTSUPP;
 				goto err_undo_flags;
 			}

-- 
2.27.0

On Tue, Aug 10, 2021 at 02:40:31AM -0400, Jonathan Toppins wrote:

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

v2:
 - changed the printks to reduce object code slightly
 - emit a single error message based on if netlink or sysfs is
   attempting to enslave

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

Can you please resubmit whole series and not as a reply and put your changelog under ---?
We don't want to see chengelog in final commit message.

Thanks

Two small patches removing unreferenced symbols and unifying error
messages across netlink and printk.

Signed-off-by: Jonathan Toppins <redacted>

Jonathan Toppins (2):
  bonding: remove extraneous definitions from bonding.h
  bonding: combine netlink and console error messages

 drivers/net/bonding/bond_main.c | 69 ++++++++++++++++++---------------
 include/net/bonding.h           | 12 ------
 2 files changed, 37 insertions(+), 44 deletions(-)

-- 
2.27.0

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>
---

Notes:
    v2:
     - changed the printks to reduce object code slightly
     - emit a single error message based on if netlink or sysfs is
       attempting to enslave
     - rebase on top of net-next/master and convert additional
       instances added by XDP additions

 drivers/net/bonding/bond_main.c | 69 ++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 365953e8013e..c0db4e2b2462 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1725,6 +1725,20 @@ void bond_lower_state_changed(struct slave *slave)
 	netdev_lower_state_changed(slave->dev, &info);
 }
 
+#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
+	if (extack)						\
+		NL_SET_ERR_MSG(extack, errmsg);			\
+	else							\
+		netdev_err(bond_dev, "Error: %s\n", errmsg);	\
+} while (0)
+
+#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: %s\n", errmsg);	\
+} while (0)
+
 /* enslave device <slave> to bond device <master> */
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		 struct netlink_ext_ack *extack)

@@ -1738,9 +1752,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	if (slave_dev->flags & IFF_MASTER &&
 	    !netif_is_bond_master(slave_dev)) {
-		NL_SET_ERR_MSG(extack, "Device with IFF_MASTER cannot be enslaved");
-		netdev_err(bond_dev,
-			   "Error: Device with IFF_MASTER cannot be enslaved\n");
+		BOND_NL_ERR(bond_dev, extack,
+			    "Device with IFF_MASTER cannot be enslaved");
 		return -EPERM;
 	}
 

@@ -1752,15 +1765,13 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 	/* already in-use? */
 	if (netdev_is_rx_handler_busy(slave_dev)) {
-		NL_SET_ERR_MSG(extack, "Device is in use and cannot be enslaved");
-		slave_err(bond_dev, slave_dev,
-			  "Error: Device is in use and cannot be enslaved\n");
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device is in use and cannot be enslaved");
 		return -EBUSY;
 	}
 
 	if (bond_dev == slave_dev) {
-		NL_SET_ERR_MSG(extack, "Cannot enslave bond to itself.");
-		netdev_err(bond_dev, "cannot enslave bond to itself.\n");
+		BOND_NL_ERR(bond_dev, extack, "Cannot enslave bond to itself.");
 		return -EPERM;
 	}
 

@@ -1769,8 +1780,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) {
 		slave_dbg(bond_dev, slave_dev, "is NETIF_F_VLAN_CHALLENGED\n");
 		if (vlan_uses_dev(bond_dev)) {
-			NL_SET_ERR_MSG(extack, "Can not enslave VLAN challenged device to VLAN enabled bond");
-			slave_err(bond_dev, slave_dev, "Error: cannot enslave VLAN challenged slave on VLAN enabled bond\n");
+			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+				     "Can not enslave VLAN challenged device to VLAN enabled bond");
 			return -EPERM;
 		} else {
 			slave_warn(bond_dev, slave_dev, "enslaved VLAN challenged slave. Adding VLANs will be blocked as long as it is part of bond.\n");

@@ -1788,8 +1799,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	 * enslaving it; the old ifenslave will not.
 	 */
 	if (slave_dev->flags & IFF_UP) {
-		NL_SET_ERR_MSG(extack, "Device can not be enslaved while up");
-		slave_err(bond_dev, slave_dev, "slave is up - this may be due to an out of date ifenslave\n");
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device can not be enslaved while up");
 		return -EPERM;
 	}
 

@@ -1828,17 +1839,15 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 						 bond_dev);
 		}
 	} else if (bond_dev->type != slave_dev->type) {
-		NL_SET_ERR_MSG(extack, "Device type is different from other slaves");
-		slave_err(bond_dev, slave_dev, "ether type (%d) is different from other slaves (%d), can not enslave it\n",
-			  slave_dev->type, bond_dev->type);
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Device type is different from other slaves");
 		return -EINVAL;
 	}
 
 	if (slave_dev->type == ARPHRD_INFINIBAND &&
 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
-		NL_SET_ERR_MSG(extack, "Only active-backup mode is supported for infiniband slaves");
-		slave_warn(bond_dev, slave_dev, "Type (%d) supports only active-backup mode\n",
-			   slave_dev->type);
+		SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+			     "Only active-backup mode is supported for infiniband slaves");
 		res = -EOPNOTSUPP;
 		goto err_undo_flags;
 	}

@@ -1852,8 +1861,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 				bond->params.fail_over_mac = BOND_FOM_ACTIVE;
 				slave_warn(bond_dev, slave_dev, "Setting fail_over_mac to active for active-backup mode\n");
 			} else {
-				NL_SET_ERR_MSG(extack, "Slave device does not support setting the MAC address, but fail_over_mac is not set to active");
-				slave_err(bond_dev, slave_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n");
+				SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+					     "Slave device does not support setting the MAC address, but fail_over_mac is not set to active");
 				res = -EOPNOTSUPP;
 				goto err_undo_flags;
 			}

@@ -2149,8 +2158,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	if (!slave_dev->netdev_ops->ndo_bpf ||
 	    !slave_dev->netdev_ops->ndo_xdp_xmit) {
 		if (bond->xdp_prog) {
-			NL_SET_ERR_MSG(extack, "Slave does not support XDP");
-			slave_err(bond_dev, slave_dev, "Slave does not support XDP\n");
+			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+				     "Slave does not support XDP");
 			res = -EOPNOTSUPP;
 			goto err_sysfs_del;
 		}

@@ -2163,10 +2172,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		};
 
 		if (dev_xdp_prog_count(slave_dev) > 0) {
-			NL_SET_ERR_MSG(extack,
-				       "Slave has XDP program loaded, please unload before enslaving");
-			slave_err(bond_dev, slave_dev,
-				  "Slave has XDP program loaded, please unload before enslaving\n");
+			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
+				     "Slave has XDP program loaded, please unload before enslaving");
 			res = -EOPNOTSUPP;
 			goto err_sysfs_del;
 		}

@@ -5190,17 +5197,15 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 		if (!slave_dev->netdev_ops->ndo_bpf ||
 		    !slave_dev->netdev_ops->ndo_xdp_xmit) {
-			NL_SET_ERR_MSG(extack, "Slave device does not support XDP");
-			slave_err(dev, slave_dev, "Slave does not support XDP\n");
+			SLAVE_NL_ERR(dev, slave_dev, extack,
+				     "Slave device does not support XDP");
 			err = -EOPNOTSUPP;
 			goto err;
 		}
 
 		if (dev_xdp_prog_count(slave_dev) > 0) {
-			NL_SET_ERR_MSG(extack,
-				       "Slave has XDP program loaded, please unload before enslaving");
-			slave_err(dev, slave_dev,
-				  "Slave has XDP program loaded, please unload before enslaving\n");
+			SLAVE_NL_ERR(dev, slave_dev, extack,
+				     "Slave has XDP program loaded, please unload before enslaving");
 			err = -EOPNOTSUPP;
 			goto err;
 		}

-- 
2.27.0

All of the symbols either only exist in bond_options.c or nowhere at
all. These symbols were verified to not exist in the code base by
using `git grep` and their removal was verified by compiling bonding.ko.

Signed-off-by: Jonathan Toppins <redacted>
---
 include/net/bonding.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/include/net/bonding.h b/include/net/bonding.h
index 9f3fdc180c6c..15e083e18f75 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h

@@ -150,11 +150,6 @@ struct bond_params {
 	u8 ad_actor_system[ETH_ALEN + 2];
 };
 
-struct bond_parm_tbl {
-	char *modename;
-	int mode;
-};
-
 struct slave {
 	struct net_device *dev; /* first - useful for panic debug */
 	struct bonding *bond; /* our master */

@@ -755,13 +750,6 @@ static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
 
 /* exported from bond_main.c */
 extern unsigned int bond_net_id;
-extern const struct bond_parm_tbl bond_lacp_tbl[];
-extern const struct bond_parm_tbl xmit_hashtype_tbl[];
-extern const struct bond_parm_tbl arp_validate_tbl[];
-extern const struct bond_parm_tbl arp_all_targets_tbl[];
-extern const struct bond_parm_tbl fail_over_mac_tbl[];
-extern const struct bond_parm_tbl pri_reselect_tbl[];
-extern struct bond_parm_tbl ad_select_tbl[];
 
 /* exported from bond_netlink.c */
 extern struct rtnl_link_ops bond_link_ops;

-- 
2.27.0

On Tue, 2021-08-10 at 22:53 -0400, Jonathan Toppins wrote:

[]

Ideally both of these would be static functions and not macros.

On Tue, 10 Aug 2021 20:27:01 -0700 Joe Perches wrote:

quoted

+#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
+	if (extack)						\
+		NL_SET_ERR_MSG(extack, errmsg);			\
+	else							\
+		netdev_err(bond_dev, "Error: %s\n", errmsg);	\
+} while (0)
+
+#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: %s\n", errmsg);	\
+} while (0)  

Ideally both of these would be static functions and not macros.

That may break our ability for NL_SET_ERR_MSG to place strings 
back in a static buffer, no?

On Wed, 2021-08-11 at 05:49 -0700, Jakub Kicinski wrote:

On Tue, 10 Aug 2021 20:27:01 -0700 Joe Perches wrote:

quoted

quoted

+#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
+	if (extack)						\
+		NL_SET_ERR_MSG(extack, errmsg);			\
+	else							\
+		netdev_err(bond_dev, "Error: %s\n", errmsg);	\
+} while (0)
+
+#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: %s\n", errmsg);	\
+} while (0)  

Ideally both of these would be static functions and not macros.

That may break our ability for NL_SET_ERR_MSG to place strings 
back in a static buffer, no?

Not really.

The most common way to place things in a particular section is to
use __section("whatever")

It's pretty trivial to mark these errmsg strings as above.

On 8/11/21 9:23 AM, Joe Perches wrote:

On Wed, 2021-08-11 at 05:49 -0700, Jakub Kicinski wrote:

quoted

On Tue, 10 Aug 2021 20:27:01 -0700 Joe Perches wrote:

quoted

quoted

+#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
+	if (extack)						\
+		NL_SET_ERR_MSG(extack, errmsg);			\
+	else							\
+		netdev_err(bond_dev, "Error: %s\n", errmsg);	\
+} while (0)
+
+#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: %s\n", errmsg);	\
+} while (0)

Ideally both of these would be static functions and not macros.

That may break our ability for NL_SET_ERR_MSG to place strings
back in a static buffer, no?

Not really.

The most common way to place things in a particular section is to
use __section("whatever")

It's pretty trivial to mark these errmsg strings as above.

I am unable to convert these to functions at this time, due to how 
NL_SET_ERR_MSG is expanded. This is with either a param list prototype 
of, "const char *errmsg" or "const char errmsg[]".

$ make C=1 drivers/net/bonding/bonding.ko
   CALL    scripts/checksyscalls.sh
   CALL    scripts/atomic/check-atomics.sh
   DESCEND objtool
   DESCEND bpf/resolve_btfids
   CC [M]  drivers/net/bonding/bond_main.o
In file included from ./include/uapi/linux/neighbour.h:6,
                  from ./include/linux/netdevice.h:45,
                  from ./include/net/inet_sock.h:19,
                  from ./include/net/ip.h:28,
                  from drivers/net/bonding/bond_main.c:42:
drivers/net/bonding/bond_main.c: In function ‘bond_nl_err’:
drivers/net/bonding/bond_main.c:1733:26: error: invalid initializer
    NL_SET_ERR_MSG(extack, errmsg);
                           ^~~~~~
./include/linux/netlink.h:92:30: note: in definition of macro 
‘NL_SET_ERR_MSG’
   static const char __msg[] = msg;  \
                               ^~~
drivers/net/bonding/bond_main.c: In function ‘slave_nl_err’:
drivers/net/bonding/bond_main.c:1744:26: error: invalid initializer
    NL_SET_ERR_MSG(extack, errmsg);
                           ^~~~~~
./include/linux/netlink.h:92:30: note: in definition of macro 
‘NL_SET_ERR_MSG’
   static const char __msg[] = msg;  \
                               ^~~
make[3]: *** [scripts/Makefile.build:271: 
drivers/net/bonding/bond_main.o] Error 1
make[2]: *** [scripts/Makefile.build:514: drivers/net/bonding] Error 2
make[1]: *** [scripts/Makefile.build:514: drivers/net] Error 2
make: *** [Makefile:1841: drivers] Error 2

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue, 10 Aug 2021 22:53:29 -0400 you wrote:

Two small patches removing unreferenced symbols and unifying error
messages across netlink and printk.

Signed-off-by: Jonathan Toppins <redacted>

Jonathan Toppins (2):
  bonding: remove extraneous definitions from bonding.h
  bonding: combine netlink and console error messages

[...]

Here is the summary with links:
  - [net-next,v2,1/2] bonding: remove extraneous definitions from bonding.h
    https://git.kernel.org/netdev/net-next/c/891a88f4f576
  - [net-next,v2,2/2] bonding: combine netlink and console error messages
    https://git.kernel.org/netdev/net-next/c/6569fa2d4e01

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html