[PATCH net-next 1/2] bonding: remove extraneous definitions from bonding.h

[PATCH net-next 1/2] bonding: remove extraneous definitions from bonding.h

[Jonathan Toppins]

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 <jtoppins@redhat.com>
---
 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

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

[Jonathan Toppins]

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 <jtoppins@redhat.com>
---
 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

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

[Joe Perches]

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.
[]
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
[]
> +#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)

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

[Jonathan Toppins]

On 8/6/21 11:52 PM, Joe Perches wrote:
> 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.
> []
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> []
>> +#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 <jtoppins@redhat.com>
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.

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

[Joe Perches]

On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:
> On 8/6/21 11:52 PM, Joe Perches wrote:
> > 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.
> > []
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > []
> > > +#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.

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

[Jonathan Toppins]

On 8/8/21 6:02 AM, Joe Perches wrote:
> On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:
>> On 8/6/21 11:52 PM, Joe Perches wrote:
>>> 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.
>>> []
>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> []
>>>> +#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 <jtoppins@redhat.com>
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

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

[Joe Perches]

On Sun, 2021-08-08 at 22:07 -0400, Jonathan Toppins wrote:
> On 8/8/21 6:02 AM, Joe Perches wrote:
> > On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:
> > > On 8/6/21 11:52 PM, Joe Perches wrote:
> > > > 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.
> > > > []
> > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > > []
> > > > > +#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 <jtoppins@redhat.com>
> 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 <broonie@kernel.org>
Date:   Fri Aug 6 17:52:53 2021 +0100

    Add linux-next specific files for 20210806
    
    Signed-off-by: Mark Brown <broonie@kernel.org>

$ 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

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

[Jonathan Toppins]

On 8/9/21 1:05 AM, Joe Perches wrote:
> On Sun, 2021-08-08 at 22:07 -0400, Jonathan Toppins wrote:
>> On 8/8/21 6:02 AM, Joe Perches wrote:
>>> On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:
>>>> On 8/6/21 11:52 PM, Joe Perches wrote:
>>>>> 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.
>>>>> []
>>>>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>>> []
>>>>>> +#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 <jtoppins@redhat.com>
>> 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 <broonie@kernel.org>
> Date:   Fri Aug 6 17:52:53 2021 +0100
> 
>      Add linux-next specific files for 20210806
>      
>      Signed-off-by: Mark Brown <broonie@kernel.org>
> 
> $ 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

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

[Leon Romanovsky]

On Fri, Aug 06, 2021 at 11:30:55PM -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.
> 
> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com>
> ---
>  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.

Thanks

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

[Jonathan Toppins]

On 8/8/21 6:16 AM, Leon Romanovsky wrote:
> On Fri, Aug 06, 2021 at 11:30:55PM -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.
>>
>> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com>
>> ---
>>   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

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

[Leon Romanovsky]

On Sun, Aug 08, 2021 at 09:42:46PM -0400, Jonathan Toppins wrote:
> On 8/8/21 6:16 AM, Leon Romanovsky wrote:
> > On Fri, Aug 06, 2021 at 11:30:55PM -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.
> > > 
> > > Signed-off-by: Jonathan Toppins <jtoppins@redhat.com>
> > > ---
> > >   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
> 

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

[Jonathan Toppins]

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 <jtoppins@redhat.com>
---
 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

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

[Leon Romanovsky]

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 <jtoppins@redhat.com>
> ---
>  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