| Message ID | e6b78ce8f5904a5411a809cf4205d745f8af98cb.1628650079.git.jtoppins@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6569fa2d4e0147759f7580cb36fd6505feab12f7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bonding: cleanup header file and error msgs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 143 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
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.
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?
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.
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
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; }
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(-)