* [RFC net-next] bonding: netlink error message support for options
@ 2022-05-17 20:31 Jonathan Toppins
2022-05-17 21:11 ` Jay Vosburgh
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jonathan Toppins @ 2022-05-17 20:31 UTC (permalink / raw)
To: netdev
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
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.
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC net-next] bonding: netlink error message support for options
2022-05-17 20:31 [RFC net-next] bonding: netlink error message support for options Jonathan Toppins
@ 2022-05-17 21:11 ` Jay Vosburgh
2022-05-17 22:33 ` Jakub Kicinski
2022-05-17 22:44 ` Stephen Hemminger
2022-05-27 19:59 ` [RFC net-next v2] " Jonathan Toppins
2 siblings, 1 reply; 7+ messages in thread
From: Jay Vosburgh @ 2022-05-17 21:11 UTC (permalink / raw)
To: Jonathan Toppins
Cc: netdev, Veaceslav Falico, Andy Gospodarek, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC net-next] bonding: netlink error message support for options
2022-05-17 21:11 ` Jay Vosburgh
@ 2022-05-17 22:33 ` Jakub Kicinski
0 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2022-05-17 22:33 UTC (permalink / raw)
To: Jay Vosburgh
Cc: Jonathan Toppins, netdev, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Eric Dumazet, Paolo Abeni, linux-kernel
On Tue, 17 May 2022 14:11:14 -0700 Jay Vosburgh wrote:
> 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));
> }
Indeed.
> Even if the strings have to be constant (via NL_SET_ERR_MSG),
> adding extack messages is likely still an improvement.
At a quick glance it seems like the major use of the printf here is to
point at a particular option. If options are carried in individual
attributes pointing at the right attribute with NL_SET_ERR_MSG_ATTR()
should also be helpful. Maybe that's stating the obvious.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC net-next] bonding: netlink error message support for options
2022-05-17 20:31 [RFC net-next] bonding: netlink error message support for options Jonathan Toppins
2022-05-17 21:11 ` Jay Vosburgh
@ 2022-05-17 22:44 ` Stephen Hemminger
2022-05-17 23:54 ` Jakub Kicinski
2022-05-27 19:59 ` [RFC net-next v2] " Jonathan Toppins
2 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2022-05-17 22:44 UTC (permalink / raw)
To: Jonathan Toppins
Cc: netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
On Tue, 17 May 2022 16:31:19 -0400
Jonathan Toppins <jtoppins@redhat.com> wrote:
> 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.
Might be better for others in long term if NL_SET_ERR_MSG() had printf like
semantics. Surely this isn't going to be first or last case.
Then internally, it could print right to the netlink message.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC net-next] bonding: netlink error message support for options
2022-05-17 22:44 ` Stephen Hemminger
@ 2022-05-17 23:54 ` Jakub Kicinski
2022-05-18 3:37 ` Jonathan Toppins
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2022-05-17 23:54 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jonathan Toppins, netdev, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Eric Dumazet, Paolo Abeni,
linux-kernel
On Tue, 17 May 2022 15:44:19 -0700 Stephen Hemminger wrote:
> On Tue, 17 May 2022 16:31:19 -0400
> Jonathan Toppins <jtoppins@redhat.com> wrote:
>
> > 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.
>
> Might be better for others in long term if NL_SET_ERR_MSG() had printf like
> semantics. Surely this isn't going to be first or last case.
>
> Then internally, it could print right to the netlink message.
Dunno. I think pointing at the bad attr + exposing per-attr netlink
parsing policy + a string for a human worked pretty well so far.
IMHO printf() is just a knee jerk reaction, especially when converting
from netdev_err().
Augmenting structured information is much, much better long term.
To me the never ending stream of efforts to improve printk() is a
proof that once we let people printf() at will, efforts to contain
it will be futile.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC net-next] bonding: netlink error message support for options
2022-05-17 23:54 ` Jakub Kicinski
@ 2022-05-18 3:37 ` Jonathan Toppins
0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Toppins @ 2022-05-18 3:37 UTC (permalink / raw)
To: Jakub Kicinski, Stephen Hemminger
Cc: netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Eric Dumazet, Paolo Abeni, linux-kernel
On 5/17/22 19:54, Jakub Kicinski wrote:
> On Tue, 17 May 2022 15:44:19 -0700 Stephen Hemminger wrote:
>> On Tue, 17 May 2022 16:31:19 -0400
>> Jonathan Toppins <jtoppins@redhat.com> wrote:
>>
>>> 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.
>>
>> Might be better for others in long term if NL_SET_ERR_MSG() had printf like
>> semantics. Surely this isn't going to be first or last case.
>>
>> Then internally, it could print right to the netlink message.
>
> Dunno. I think pointing at the bad attr + exposing per-attr netlink
> parsing policy + a string for a human worked pretty well so far.
> IMHO printf() is just a knee jerk reaction, especially when converting
> from netdev_err().
For some subsystems it is not a convert from netdev_err, it is an AND.
In this RFC there are instances where changing the message from
netdev_err() to the macro was trivial;
@@ -240,12 +243,14 @@ static int bond_changelink(struct net_device
*bond_dev, st
ruct 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;
}
These are trivial because the path does not have to care about sysfs or
some other legacy configuration interface. These macros become rather
annoying to use once a system needs to support multiple configuration
paths and is trying to utilize as much common configuration code[0] as
possible so that all interfaces largely operate the same way.
>
> Augmenting structured information is much, much better long term.
>
> To me the never ending stream of efforts to improve printk() is a
> proof that once we let people printf() at will, efforts to contain
> it will be futile.
>
At least for bonding I was trying to reuse the most amount of code which
needs to deal with both sysfs and netlink. And I don't think it is a
good idea to split the code paths, so if I am suppose to use statically
allocated strings to support netlink errors that basically means
anything that has to support multiple interfaces gets to sprinkle `if
(extack)` everywhere[0]. Not great. The ownership model of the error
buffer seems odd to me with the current macros, I am suppose to set a
pointer in a structure subsystem X didn't allocate and has no control
over its lifetime. Then netlink takes this pointer and does whatever
with it. And somehow subsystem X is suppose to guarantee the pointer's
lifetime exists forever, making a `const static char[]` buffer the only
option. I don't understand why netlink doesn't provide the buffer and a
subsystem just populates it. Using memcpy or snprintf doesn't matter, to
me its a lifetime issue that makes the API not great to work with when
you have to handle cases other than netlink.
Also as Joe Perches points out in this thread[1,2] the way the macros
are written it is bloating the kernel because the error messages are
getting duplicated for subsystems that need to support multiple
configuration interfaces.
-Jon
[0]
https://lore.kernel.org/netdev/e6b78ce8f5904a5411a809cf4205d745f8af98cb.1628650079.git.jtoppins@redhat.com/
[1] https://lore.kernel.org/netdev/cover.1628306392.git.jtoppins@redhat.com/
[2]
https://lore.kernel.org/netdev/c8b69905c995ab887633ef11862705ee66c60aad.camel@perches.com/
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC net-next v2] bonding: netlink error message support for options
2022-05-17 20:31 [RFC net-next] bonding: netlink error message support for options Jonathan Toppins
2022-05-17 21:11 ` Jay Vosburgh
2022-05-17 22:44 ` Stephen Hemminger
@ 2022-05-27 19:59 ` Jonathan Toppins
2 siblings, 0 replies; 7+ messages in thread
From: Jonathan Toppins @ 2022-05-27 19:59 UTC (permalink / raw)
To: netdev
Cc: jtoppins, andy, davem, edumazet, j.vosburgh, kuba, linux-kernel,
pabeni, vfalico
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: unable to set option because the bond is up.
Signed-off-by: Jonathan Toppins <jtoppins@redhat.com>
---
Notes:
v2:
Removed the printf support and just added static messages for various
error events.
drivers/net/bonding/bond_netlink.c | 101 +++++++++++++++++++----------
drivers/net/bonding/bond_options.c | 29 +++++++--
include/net/bond_options.h | 3 +-
3 files changed, 92 insertions(+), 41 deletions(-)
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index f427fa1737c7..565ee0ac3e60 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -151,7 +151,8 @@ 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,
+ data[IFLA_BOND_SLAVE_QUEUE_ID], extack);
if (err)
return err;
}
@@ -175,7 +176,8 @@ 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,
+ data[IFLA_BOND_MODE], extack);
if (err)
return err;
}
@@ -192,7 +194,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,
+ data[IFLA_BOND_ACTIVE_SLAVE], extack);
if (err)
return err;
}
@@ -200,7 +203,8 @@ 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,
+ data[IFLA_BOND_MIIMON], extack);
if (err)
return err;
}
@@ -208,7 +212,8 @@ 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,
+ data[IFLA_BOND_UPDELAY], extack);
if (err)
return err;
}
@@ -216,7 +221,8 @@ 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,
+ data[IFLA_BOND_DOWNDELAY], extack);
if (err)
return err;
}
@@ -224,7 +230,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,
+ data[IFLA_BOND_PEER_NOTIF_DELAY], extack);
if (err)
return err;
}
@@ -232,7 +239,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,
+ data[IFLA_BOND_USE_CARRIER], extack);
if (err)
return err;
}
@@ -240,12 +248,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_ATTR(extack, data[IFLA_BOND_ARP_INTERVAL],
+ "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,
+ data[IFLA_BOND_ARP_INTERVAL], extack);
if (err)
return err;
}
@@ -264,7 +274,9 @@ 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,
+ data[IFLA_BOND_ARP_IP_TARGET],
+ extack);
if (err)
break;
i++;
@@ -297,7 +309,9 @@ 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,
+ data[IFLA_BOND_NS_IP6_TARGET],
+ extack);
if (err)
break;
i++;
@@ -312,12 +326,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_ATTR(extack, data[IFLA_BOND_ARP_INTERVAL],
+ "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,
+ data[IFLA_BOND_ARP_VALIDATE], extack);
if (err)
return err;
}
@@ -326,7 +342,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,
+ data[IFLA_BOND_ARP_ALL_TARGETS], extack);
if (err)
return err;
}
@@ -340,7 +357,8 @@ 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,
+ data[IFLA_BOND_PRIMARY], extack);
if (err)
return err;
}
@@ -349,7 +367,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,
+ data[IFLA_BOND_PRIMARY_RESELECT], extack);
if (err)
return err;
}
@@ -358,7 +377,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,
+ data[IFLA_BOND_FAIL_OVER_MAC], extack);
if (err)
return err;
}
@@ -367,7 +387,8 @@ 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,
+ data[IFLA_BOND_XMIT_HASH_POLICY], extack);
if (err)
return err;
}
@@ -376,7 +397,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,
+ data[IFLA_BOND_RESEND_IGMP], extack);
if (err)
return err;
}
@@ -385,7 +407,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,
+ data[IFLA_BOND_NUM_PEER_NOTIF], extack);
if (err)
return err;
}
@@ -394,7 +417,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,
+ data[IFLA_BOND_ALL_SLAVES_ACTIVE], extack);
if (err)
return err;
}
@@ -403,7 +427,8 @@ 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,
+ data[IFLA_BOND_MIN_LINKS], extack);
if (err)
return err;
}
@@ -412,7 +437,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,
+ data[IFLA_BOND_LP_INTERVAL], extack);
if (err)
return err;
}
@@ -421,7 +447,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,
+ data[IFLA_BOND_PACKETS_PER_SLAVE], extack);
if (err)
return err;
}
@@ -430,7 +457,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,
+ data[IFLA_BOND_AD_LACP_ACTIVE], extack);
if (err)
return err;
}
@@ -440,7 +468,8 @@ 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,
+ data[IFLA_BOND_AD_LACP_RATE], extack);
if (err)
return err;
}
@@ -449,7 +478,8 @@ 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,
+ data[IFLA_BOND_AD_SELECT], extack);
if (err)
return err;
}
@@ -458,7 +488,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,
+ data[IFLA_BOND_AD_ACTOR_SYS_PRIO], extack);
if (err)
return err;
}
@@ -467,7 +498,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,
+ data[IFLA_BOND_AD_USER_PORT_KEY], extack);
if (err)
return err;
}
@@ -477,7 +509,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,
+ data[IFLA_BOND_AD_ACTOR_SYSTEM], extack);
if (err)
return err;
}
@@ -485,7 +518,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,
+ data[IFLA_BOND_TLB_DYNAMIC_LB], extack);
if (err)
return err;
}
@@ -494,7 +528,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,
+ data[IFLA_BOND_MISSED_MAX], extack);
if (err)
return err;
}
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 64f7db2627ce..36945a513770 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -636,27 +636,35 @@ 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,
+ struct nlattr *bad_attr,
+ struct netlink_ext_ack *extack)
{
const struct bond_opt_value *modeval;
struct bond_params *params;
params = &bond->params;
modeval = bond_opt_get_val(BOND_OPT_MODE, params->mode);
- if (test_bit(params->mode, &opt->unsuppmodes))
+ 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);
+ NL_SET_ERR_MSG_ATTR(extack, bad_attr,
+ "option not supported in mode");
+ }
}
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,
+ struct nlattr *bad_attr,
+ struct netlink_ext_ack *extack)
{
const struct bond_opt_value *minval, *maxval;
char *p;
switch (error) {
case -EINVAL:
+ NL_SET_ERR_MSG_ATTR(extack, bad_attr, "invalid option value");
if (val) {
if (val->string) {
/* sometimes RAWVAL opts may have new lines */
@@ -678,13 +686,17 @@ static void bond_opt_error_interpret(struct bonding *bond,
opt->name, minval ? minval->value : 0, maxval->value);
break;
case -EACCES:
- bond_opt_dep_print(bond, opt);
+ bond_opt_dep_print(bond, opt, bad_attr, extack);
break;
case -ENOTEMPTY:
+ NL_SET_ERR_MSG_ATTR(extack, bad_attr,
+ "unable to set option because the bond device has slaves");
netdev_err(bond->dev, "option %s: unable to set because the bond device has slaves\n",
opt->name);
break;
case -EBUSY:
+ NL_SET_ERR_MSG_ATTR(extack, bad_attr,
+ "unable to set option because the bond is up");
netdev_err(bond->dev, "option %s: unable to set because the bond device is up\n",
opt->name);
break;
@@ -695,6 +707,8 @@ static void bond_opt_error_interpret(struct bonding *bond,
*p = '\0';
netdev_err(bond->dev, "option %s: interface %s does not exist!\n",
opt->name, val->string);
+ NL_SET_ERR_MSG_ATTR(extack, bad_attr,
+ "interface does not exist");
}
break;
default:
@@ -713,7 +727,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 nlattr *bad_attr, struct netlink_ext_ack *extack)
{
const struct bond_opt_value *retval = NULL;
const struct bond_option *opt;
@@ -735,7 +750,7 @@ int __bond_opt_set(struct bonding *bond,
ret = opt->set(bond, retval);
out:
if (ret)
- bond_opt_error_interpret(bond, opt, ret, val);
+ bond_opt_error_interpret(bond, opt, ret, val, bad_attr, extack);
return ret;
}
@@ -757,7 +772,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, 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..1618b76f4903 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -107,7 +107,8 @@ struct bond_option {
};
int __bond_opt_set(struct bonding *bond, unsigned int option,
- struct bond_opt_value *val);
+ struct bond_opt_value *val,
+ struct nlattr *bad_attr, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-27 20:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 20:31 [RFC net-next] bonding: netlink error message support for options Jonathan Toppins
2022-05-17 21:11 ` Jay Vosburgh
2022-05-17 22:33 ` Jakub Kicinski
2022-05-17 22:44 ` Stephen Hemminger
2022-05-17 23:54 ` Jakub Kicinski
2022-05-18 3:37 ` Jonathan Toppins
2022-05-27 19:59 ` [RFC net-next v2] " Jonathan Toppins
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).