Re: [PATCH net-next] Bonding: add per port priority support

[Hangbin Liu <liuhangbin@gmail.com>]

On Tue, Apr 12, 2022 at 08:55:41AM -0700, Jay Vosburgh wrote:
> >> @@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> >>   				 struct nlattr *tb[], struct nlattr *data[],
> >>   				 struct netlink_ext_ack *extack)
> >>   {
> >> +	struct slave *slave = bond_slave_get_rtnl(slave_dev);
> >>   	struct bonding *bond = netdev_priv(bond_dev);
> >>   	struct bond_opt_value newval;
> >>   	int err;
> >> @@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> >>   			return err;
> >>   	}
> >>   +	/* No need to bother __bond_opt_set as we only support netlink
> >> config */
> >
> >Not sure this comment is necessary, it doesn't add any value. Also I would
> >recommend using bonding's options management, as it would allow for
> >checking if the value is in a defined range. That might not be
> >particularly useful in this context since it appears +/-INT_MAX is the
> >range.
> 
> 	Agreed, on both the comment and in regards to using the extant
> bonding options management stuff.
> 
> >Also, in the Documentation it is mentioned that this parameter is only
> >used in modes active-backup and balance-alb/tlb. Do we need to send an
> >error message back preventing the modification of this value when not in
> >these modes?
> 
> 	Using the option management stuff would get this for free.

Hi Jav, Jon,

I remembered the reason why I didn't use bond default option management.

It's because the bonding options management only take bond and values. We
need to create an extra string to save the slave name and option values.
Then in bond option setting function we extract the info from the string
and do setting again, like the bond_option_queue_id_set().

I think this is too heavy for just an int value setting for slave.
As we only support netlink for new options. There is no need to handle
string setting via sysfs. For mode checking, we do just do like:

if (!bond_uses_primary(bond))
	return -EACCES;

So why bother the bonding options management? What do you think?
Do you have a easier way to get the slave name in options management?
If yes, I'm happy to use the default option management.

Thanks
Hangbin
> 
> 	-J
> 
> >> +	if (data[IFLA_BOND_SLAVE_PRIO]) {
> >> +		slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> >> +		bond_select_active_slave(bond);
> >> +	}
> >> +
> >>   	return 0;