* [PATCH net-next v2 1/2] bonding: remove extraneous definitions from bonding.h [not found] <cover.1628650079.git.jtoppins@redhat.com> @ 2021-08-11 2:53 ` Jonathan Toppins 2021-08-11 2:53 ` [PATCH net-next v2 2/2] bonding: combine netlink and console error messages Jonathan Toppins 1 sibling, 0 replies; 8+ messages in thread From: Jonathan Toppins @ 2021-08-11 2:53 UTC (permalink / raw) To: netdev, joe, leon Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, Jakub Kicinski, linux-kernel 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 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next v2 2/2] bonding: combine netlink and console error messages [not found] <cover.1628650079.git.jtoppins@redhat.com> 2021-08-11 2:53 ` [PATCH net-next v2 1/2] bonding: remove extraneous definitions from bonding.h Jonathan Toppins @ 2021-08-11 2:53 ` Jonathan Toppins 2021-08-11 3:27 ` Joe Perches 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Toppins @ 2021-08-11 2:53 UTC (permalink / raw) To: netdev, joe, leon Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, Jakub Kicinski, linux-kernel 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> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/2] bonding: combine netlink and console error messages 2021-08-11 2:53 ` [PATCH net-next v2 2/2] bonding: combine netlink and console error messages Jonathan Toppins @ 2021-08-11 3:27 ` Joe Perches 2021-08-11 12:49 ` Jakub Kicinski 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2021-08-11 3:27 UTC (permalink / raw) To: Jonathan Toppins, netdev, leon Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, Jakub Kicinski, linux-kernel On Tue, 2021-08-10 at 22:53 -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> > --- > > 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 [] > @@ -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) Ideally both of these would be static functions and not macros. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/2] bonding: combine netlink and console error messages 2021-08-11 3:27 ` Joe Perches @ 2021-08-11 12:49 ` Jakub Kicinski 2021-08-11 13:23 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Jakub Kicinski @ 2021-08-11 12:49 UTC (permalink / raw) To: Joe Perches Cc: Jonathan Toppins, netdev, leon, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, linux-kernel On Tue, 10 Aug 2021 20:27:01 -0700 Joe Perches wrote: > > +#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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/2] bonding: combine netlink and console error messages 2021-08-11 12:49 ` Jakub Kicinski @ 2021-08-11 13:23 ` Joe Perches 2021-08-11 14:31 ` Jonathan Toppins 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2021-08-11 13:23 UTC (permalink / raw) To: Jakub Kicinski Cc: Jonathan Toppins, netdev, leon, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, linux-kernel On Wed, 2021-08-11 at 05:49 -0700, Jakub Kicinski wrote: > On Tue, 10 Aug 2021 20:27:01 -0700 Joe Perches wrote: > > > +#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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/2] bonding: combine netlink and console error messages 2021-08-11 13:23 ` Joe Perches @ 2021-08-11 14:31 ` Jonathan Toppins 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Toppins @ 2021-08-11 14:31 UTC (permalink / raw) To: Joe Perches, Jakub Kicinski Cc: netdev, leon, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, linux-kernel On 8/11/21 9:23 AM, Joe Perches wrote: > On Wed, 2021-08-11 at 05:49 -0700, Jakub Kicinski wrote: >> On Tue, 10 Aug 2021 20:27:01 -0700 Joe Perches wrote: >>>> +#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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 2/2] bonding: combine netlink and console error messages @ 2021-08-07 3:30 Jonathan Toppins 2021-08-10 6:40 ` [PATCH net-next v2 " Jonathan Toppins 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Toppins @ 2021-08-07 3:30 UTC (permalink / raw) To: netdev Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, Jakub Kicinski, linux-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next v2 2/2] bonding: combine netlink and console error messages 2021-08-07 3:30 [PATCH net-next " Jonathan Toppins @ 2021-08-10 6:40 ` Jonathan Toppins 2021-08-10 6:47 ` Leon Romanovsky 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Toppins @ 2021-08-10 6:40 UTC (permalink / raw) To: netdev, joe, leon Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, Jakub Kicinski, linux-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2 2/2] bonding: combine netlink and console error messages 2021-08-10 6:40 ` [PATCH net-next v2 " Jonathan Toppins @ 2021-08-10 6:47 ` Leon Romanovsky 0 siblings, 0 replies; 8+ messages in thread From: Leon Romanovsky @ 2021-08-10 6:47 UTC (permalink / raw) To: Jonathan Toppins Cc: netdev, joe, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller, Jakub Kicinski, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-08-11 14:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1628650079.git.jtoppins@redhat.com>
2021-08-11 2:53 ` [PATCH net-next v2 1/2] bonding: remove extraneous definitions from bonding.h Jonathan Toppins
2021-08-11 2:53 ` [PATCH net-next v2 2/2] bonding: combine netlink and console error messages Jonathan Toppins
2021-08-11 3:27 ` Joe Perches
2021-08-11 12:49 ` Jakub Kicinski
2021-08-11 13:23 ` Joe Perches
2021-08-11 14:31 ` Jonathan Toppins
2021-08-07 3:30 [PATCH net-next " Jonathan Toppins
2021-08-10 6:40 ` [PATCH net-next v2 " Jonathan Toppins
2021-08-10 6:47 ` Leon Romanovsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).