Messages in this thread Patch in this message | | | From | Jay Vosburgh <> | Subject | Re: [RFC net-next] bonding: netlink error message support for options | Date | Tue, 17 May 2022 14:11:14 -0700 |
| |
Jonathan Toppins <jtoppins@redhat.com> wrote:
>Add support for reporting errors via extack in both bond_newlink >and bond_changelink. > >Instead of having to look in the kernel log for why an option was not >correct just report the error to the user via the extack variable. > >What is currently reported today: > ip link add bond0 type bond > ip link set bond0 up > ip link set bond0 type bond mode 4 > RTNETLINK answers: Device or resource busy > >After this change: > ip link add bond0 type bond > ip link set bond0 up > ip link set bond0 type bond mode 4 > Error: option mode: unable to set because the bond device is up. > >Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >--- > >Notes: > This is an RFC because the current NL_SET_ERR_MSG() macros do not support > printf like semantics so I rolled my own buffer setting in __bond_opt_set(). > The issue is I could not quite figure out the life-cycle of the buffer, if > rtnl lock is held until after the text buffer is copied into the packet > then we are ok, otherwise, some other type of buffer management scheme will > be needed as this could result in corrupted error messages when modifying > multiple bonds.
If I'm reading the code correctly, rtnl isn't held that long. Once the ->doit() returns, rtnl is dropped, but the copy happens later:
rtnetlink_rcv() netlink_rcv_skb(skb, &rtnetlink_rcv_msg) rtnetlink_rcv_msg() [ as cb(skb, nlh, &extack) ] rtnl_lock() link->doit() [ rtnl_setlink, rtnl_newlink, et al ] rtnl_unlock() netlink_ack()
inside netlink_ack():
if (nlk_has_extack && extack) { if (extack->_msg) { WARN_ON(nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg)); }
Even if the strings have to be constant (via NL_SET_ERR_MSG), adding extack messages is likely still an improvement.
-J
> drivers/net/bonding/bond_netlink.c | 87 ++++++++++++++++++------------ > drivers/net/bonding/bond_options.c | 62 +++++++++++++-------- > include/net/bond_options.h | 2 +- > 3 files changed, 96 insertions(+), 55 deletions(-) > >diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c >index f427fa1737c7..418a4f3d00a3 100644 >--- a/drivers/net/bonding/bond_netlink.c >+++ b/drivers/net/bonding/bond_netlink.c >@@ -151,7 +151,7 @@ static int bond_slave_changelink(struct net_device *bond_dev, > snprintf(queue_id_str, sizeof(queue_id_str), "%s:%u\n", > slave_dev->name, queue_id); > bond_opt_initstr(&newval, queue_id_str); >- err = __bond_opt_set(bond, BOND_OPT_QUEUE_ID, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_QUEUE_ID, &newval, extack); > if (err) > return err; > } >@@ -175,7 +175,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int mode = nla_get_u8(data[IFLA_BOND_MODE]); > > bond_opt_initval(&newval, mode); >- err = __bond_opt_set(bond, BOND_OPT_MODE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_MODE, &newval, extack); > if (err) > return err; > } >@@ -192,7 +192,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > active_slave = slave_dev->name; > } > bond_opt_initstr(&newval, active_slave); >- err = __bond_opt_set(bond, BOND_OPT_ACTIVE_SLAVE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_ACTIVE_SLAVE, &newval, >+ extack); > if (err) > return err; > } >@@ -200,7 +201,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > miimon = nla_get_u32(data[IFLA_BOND_MIIMON]); > > bond_opt_initval(&newval, miimon); >- err = __bond_opt_set(bond, BOND_OPT_MIIMON, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_MIIMON, &newval, extack); > if (err) > return err; > } >@@ -208,7 +209,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int updelay = nla_get_u32(data[IFLA_BOND_UPDELAY]); > > bond_opt_initval(&newval, updelay); >- err = __bond_opt_set(bond, BOND_OPT_UPDELAY, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_UPDELAY, &newval, extack); > if (err) > return err; > } >@@ -216,7 +217,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int downdelay = nla_get_u32(data[IFLA_BOND_DOWNDELAY]); > > bond_opt_initval(&newval, downdelay); >- err = __bond_opt_set(bond, BOND_OPT_DOWNDELAY, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_DOWNDELAY, &newval, extack); > if (err) > return err; > } >@@ -224,7 +225,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int delay = nla_get_u32(data[IFLA_BOND_PEER_NOTIF_DELAY]); > > bond_opt_initval(&newval, delay); >- err = __bond_opt_set(bond, BOND_OPT_PEER_NOTIF_DELAY, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_PEER_NOTIF_DELAY, &newval, >+ extack); > if (err) > return err; > } >@@ -232,7 +234,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int use_carrier = nla_get_u8(data[IFLA_BOND_USE_CARRIER]); > > bond_opt_initval(&newval, use_carrier); >- err = __bond_opt_set(bond, BOND_OPT_USE_CARRIER, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_USE_CARRIER, &newval, >+ extack); > if (err) > return err; > } >@@ -240,12 +243,14 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int arp_interval = nla_get_u32(data[IFLA_BOND_ARP_INTERVAL]); > > if (arp_interval && miimon) { >- netdev_err(bond->dev, "ARP monitoring cannot be used with MII monitoring\n"); >+ NL_SET_ERR_MSG(extack, >+ "ARP monitoring cannot be used with MII monitoring"); > return -EINVAL; > } > > bond_opt_initval(&newval, arp_interval); >- err = __bond_opt_set(bond, BOND_OPT_ARP_INTERVAL, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_ARP_INTERVAL, &newval, >+ extack); > if (err) > return err; > } >@@ -264,7 +269,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > > bond_opt_initval(&newval, (__force u64)target); > err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS, >- &newval); >+ &newval, extack); > if (err) > break; > i++; >@@ -297,7 +302,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > > bond_opt_initextra(&newval, &addr6, sizeof(addr6)); > err = __bond_opt_set(bond, BOND_OPT_NS_TARGETS, >- &newval); >+ &newval, extack); > if (err) > break; > i++; >@@ -312,12 +317,14 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int arp_validate = nla_get_u32(data[IFLA_BOND_ARP_VALIDATE]); > > if (arp_validate && miimon) { >- netdev_err(bond->dev, "ARP validating cannot be used with MII monitoring\n"); >+ NL_SET_ERR_MSG(extack, >+ "ARP validating cannot be used with MII monitoring"); > return -EINVAL; > } > > bond_opt_initval(&newval, arp_validate); >- err = __bond_opt_set(bond, BOND_OPT_ARP_VALIDATE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_ARP_VALIDATE, &newval, >+ extack); > if (err) > return err; > } >@@ -326,7 +333,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u32(data[IFLA_BOND_ARP_ALL_TARGETS]); > > bond_opt_initval(&newval, arp_all_targets); >- err = __bond_opt_set(bond, BOND_OPT_ARP_ALL_TARGETS, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_ARP_ALL_TARGETS, &newval, >+ extack); > if (err) > return err; > } >@@ -340,7 +348,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > primary = dev->name; > > bond_opt_initstr(&newval, primary); >- err = __bond_opt_set(bond, BOND_OPT_PRIMARY, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_PRIMARY, &newval, extack); > if (err) > return err; > } >@@ -349,7 +357,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_PRIMARY_RESELECT]); > > bond_opt_initval(&newval, primary_reselect); >- err = __bond_opt_set(bond, BOND_OPT_PRIMARY_RESELECT, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_PRIMARY_RESELECT, &newval, >+ extack); > if (err) > return err; > } >@@ -358,7 +367,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_FAIL_OVER_MAC]); > > bond_opt_initval(&newval, fail_over_mac); >- err = __bond_opt_set(bond, BOND_OPT_FAIL_OVER_MAC, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_FAIL_OVER_MAC, &newval, >+ extack); > if (err) > return err; > } >@@ -367,7 +377,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_XMIT_HASH_POLICY]); > > bond_opt_initval(&newval, xmit_hash_policy); >- err = __bond_opt_set(bond, BOND_OPT_XMIT_HASH, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_XMIT_HASH, &newval, extack); > if (err) > return err; > } >@@ -376,7 +386,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u32(data[IFLA_BOND_RESEND_IGMP]); > > bond_opt_initval(&newval, resend_igmp); >- err = __bond_opt_set(bond, BOND_OPT_RESEND_IGMP, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_RESEND_IGMP, &newval, >+ extack); > if (err) > return err; > } >@@ -385,7 +396,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_NUM_PEER_NOTIF]); > > bond_opt_initval(&newval, num_peer_notif); >- err = __bond_opt_set(bond, BOND_OPT_NUM_PEER_NOTIF, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_NUM_PEER_NOTIF, &newval, >+ extack); > if (err) > return err; > } >@@ -394,7 +406,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_ALL_SLAVES_ACTIVE]); > > bond_opt_initval(&newval, all_slaves_active); >- err = __bond_opt_set(bond, BOND_OPT_ALL_SLAVES_ACTIVE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_ALL_SLAVES_ACTIVE, &newval, >+ extack); > if (err) > return err; > } >@@ -403,7 +416,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u32(data[IFLA_BOND_MIN_LINKS]); > > bond_opt_initval(&newval, min_links); >- err = __bond_opt_set(bond, BOND_OPT_MINLINKS, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_MINLINKS, &newval, extack); > if (err) > return err; > } >@@ -412,7 +425,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u32(data[IFLA_BOND_LP_INTERVAL]); > > bond_opt_initval(&newval, lp_interval); >- err = __bond_opt_set(bond, BOND_OPT_LP_INTERVAL, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_LP_INTERVAL, &newval, >+ extack); > if (err) > return err; > } >@@ -421,7 +435,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u32(data[IFLA_BOND_PACKETS_PER_SLAVE]); > > bond_opt_initval(&newval, packets_per_slave); >- err = __bond_opt_set(bond, BOND_OPT_PACKETS_PER_SLAVE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_PACKETS_PER_SLAVE, &newval, >+ extack); > if (err) > return err; > } >@@ -430,7 +445,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int lacp_active = nla_get_u8(data[IFLA_BOND_AD_LACP_ACTIVE]); > > bond_opt_initval(&newval, lacp_active); >- err = __bond_opt_set(bond, BOND_OPT_LACP_ACTIVE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_LACP_ACTIVE, &newval, >+ extack); > if (err) > return err; > } >@@ -440,7 +456,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_AD_LACP_RATE]); > > bond_opt_initval(&newval, lacp_rate); >- err = __bond_opt_set(bond, BOND_OPT_LACP_RATE, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_LACP_RATE, &newval, extack); > if (err) > return err; > } >@@ -449,7 +465,7 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u8(data[IFLA_BOND_AD_SELECT]); > > bond_opt_initval(&newval, ad_select); >- err = __bond_opt_set(bond, BOND_OPT_AD_SELECT, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_AD_SELECT, &newval, extack); > if (err) > return err; > } >@@ -458,7 +474,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u16(data[IFLA_BOND_AD_ACTOR_SYS_PRIO]); > > bond_opt_initval(&newval, actor_sys_prio); >- err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYS_PRIO, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYS_PRIO, &newval, >+ extack); > if (err) > return err; > } >@@ -467,7 +484,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > nla_get_u16(data[IFLA_BOND_AD_USER_PORT_KEY]); > > bond_opt_initval(&newval, port_key); >- err = __bond_opt_set(bond, BOND_OPT_AD_USER_PORT_KEY, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_AD_USER_PORT_KEY, &newval, >+ extack); > if (err) > return err; > } >@@ -477,7 +495,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > > bond_opt_initval(&newval, > nla_get_u64(data[IFLA_BOND_AD_ACTOR_SYSTEM])); >- err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYSTEM, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYSTEM, &newval, >+ extack); > if (err) > return err; > } >@@ -485,7 +504,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int dynamic_lb = nla_get_u8(data[IFLA_BOND_TLB_DYNAMIC_LB]); > > bond_opt_initval(&newval, dynamic_lb); >- err = __bond_opt_set(bond, BOND_OPT_TLB_DYNAMIC_LB, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_TLB_DYNAMIC_LB, &newval, >+ extack); > if (err) > return err; > } >@@ -494,7 +514,8 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[], > int missed_max = nla_get_u8(data[IFLA_BOND_MISSED_MAX]); > > bond_opt_initval(&newval, missed_max); >- err = __bond_opt_set(bond, BOND_OPT_MISSED_MAX, &newval); >+ err = __bond_opt_set(bond, BOND_OPT_MISSED_MAX, &newval, >+ extack); > if (err) > return err; > } >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >index 64f7db2627ce..4a95503384a3 100644 >--- a/drivers/net/bonding/bond_options.c >+++ b/drivers/net/bonding/bond_options.c >@@ -636,7 +636,8 @@ static int bond_opt_check_deps(struct bonding *bond, > } > > static void bond_opt_dep_print(struct bonding *bond, >- const struct bond_option *opt) >+ const struct bond_option *opt, >+ char *buf, size_t bufsize) > { > const struct bond_opt_value *modeval; > struct bond_params *params; >@@ -644,16 +645,18 @@ static void bond_opt_dep_print(struct bonding *bond, > params = &bond->params; > modeval = bond_opt_get_val(BOND_OPT_MODE, params->mode); > if (test_bit(params->mode, &opt->unsuppmodes)) >- netdev_err(bond->dev, "option %s: mode dependency failed, not supported in mode %s(%llu)\n", >- opt->name, modeval->string, modeval->value); >+ scnprintf(buf, bufsize, "option %s: mode dependency failed, not supported in mode %s(%llu)\n", >+ opt->name, modeval->string, modeval->value); > } > > static void bond_opt_error_interpret(struct bonding *bond, > const struct bond_option *opt, >- int error, const struct bond_opt_value *val) >+ int error, const struct bond_opt_value *val, >+ char *buf, size_t bufsize) > { > const struct bond_opt_value *minval, *maxval; > char *p; >+ int i = 0; > > switch (error) { > case -EINVAL: >@@ -663,38 +666,45 @@ static void bond_opt_error_interpret(struct bonding *bond, > p = strchr(val->string, '\n'); > if (p) > *p = '\0'; >- netdev_err(bond->dev, "option %s: invalid value (%s)\n", >- opt->name, val->string); >+ i = scnprintf(buf, bufsize, >+ "option %s: invalid value (%s)", >+ opt->name, val->string); > } else { >- netdev_err(bond->dev, "option %s: invalid value (%llu)\n", >- opt->name, val->value); >+ i = scnprintf(buf, bufsize, >+ "option %s: invalid value (%llu)", >+ opt->name, val->value); > } > } > minval = bond_opt_get_flags(opt, BOND_VALFLAG_MIN); > maxval = bond_opt_get_flags(opt, BOND_VALFLAG_MAX); > if (!maxval) > break; >- netdev_err(bond->dev, "option %s: allowed values %llu - %llu\n", >- opt->name, minval ? minval->value : 0, maxval->value); >+ if (i) { >+ // index buf to overwirte '\n' from above >+ buf = &buf[i]; >+ bufsize -= i; >+ } >+ scnprintf(buf, bufsize, " allowed values %llu - %llu", >+ minval ? minval->value : 0, maxval->value); > break; > case -EACCES: >- bond_opt_dep_print(bond, opt); >+ bond_opt_dep_print(bond, opt, buf, bufsize); > break; > case -ENOTEMPTY: >- netdev_err(bond->dev, "option %s: unable to set because the bond device has slaves\n", >- opt->name); >+ scnprintf(buf, bufsize, "option %s: unable to set because the bond device has slaves", >+ opt->name); > break; > case -EBUSY: >- netdev_err(bond->dev, "option %s: unable to set because the bond device is up\n", >- opt->name); >+ scnprintf(buf, bufsize, "option %s: unable to set because the bond device is up", >+ opt->name); > break; > case -ENODEV: > if (val && val->string) { > p = strchr(val->string, '\n'); > if (p) > *p = '\0'; >- netdev_err(bond->dev, "option %s: interface %s does not exist!\n", >- opt->name, val->string); >+ scnprintf(buf, bufsize, "option %s: interface %s does not exist!", >+ opt->name, val->string); > } > break; > default: >@@ -713,7 +723,8 @@ static void bond_opt_error_interpret(struct bonding *bond, > * must be obtained before calling this function. > */ > int __bond_opt_set(struct bonding *bond, >- unsigned int option, struct bond_opt_value *val) >+ unsigned int option, struct bond_opt_value *val, >+ struct netlink_ext_ack *extack) > { > const struct bond_opt_value *retval = NULL; > const struct bond_option *opt; >@@ -734,8 +745,17 @@ int __bond_opt_set(struct bonding *bond, > } > ret = opt->set(bond, retval); > out: >- if (ret) >- bond_opt_error_interpret(bond, opt, ret, val); >+ if (ret) { >+ static char buf[120]; >+ buf[0] = '\0'; >+ bond_opt_error_interpret(bond, opt, ret, val, buf, sizeof(buf)); >+ if (buf[0] != '\0') { >+ if (extack) >+ extack->_msg = buf; >+ else >+ netdev_err(bond->dev, "Error: %s\n", buf); >+ } >+ } > > return ret; > } >@@ -757,7 +777,7 @@ int __bond_opt_set_notify(struct bonding *bond, > > ASSERT_RTNL(); > >- ret = __bond_opt_set(bond, option, val); >+ ret = __bond_opt_set(bond, option, val, NULL); > > if (!ret && (bond->dev->reg_state == NETREG_REGISTERED)) > call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev); >diff --git a/include/net/bond_options.h b/include/net/bond_options.h >index 61b49063791c..ae38557adc25 100644 >--- a/include/net/bond_options.h >+++ b/include/net/bond_options.h >@@ -107,7 +107,7 @@ struct bond_option { > }; > > int __bond_opt_set(struct bonding *bond, unsigned int option, >- struct bond_opt_value *val); >+ struct bond_opt_value *val, struct netlink_ext_ack *extack); > int __bond_opt_set_notify(struct bonding *bond, unsigned int option, > struct bond_opt_value *val); > int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf); >-- >2.27.0 >
--- -Jay Vosburgh, jay.vosburgh@canonical.com
| |