Re: [PATCH net-next 3/8] bonding: add downdelay netlink support

[Jay Vosburgh <fubar@us.ibm.com>]

Scott Feldman <sfeldma@cumulusnetworks.com> wrote:

>
>On Nov 7, 2013, at 7:05 AM, Jay Vosburgh <fubar@us.ibm.com> wrote:
>
>> Scott Feldman <sfeldma@cumulusnetworks.com> wrote:
>> 
>>> +int bond_option_downdelay_set(struct bonding *bond, int downdelay)
>>> +{
>>> +	if (!(bond->params.miimon)) {
>>> +		pr_err("%s: Unable to set down delay as MII monitoring is disabled\n", bond->dev->name);
>>> +		return -EPERM;
>>> +	}
>> 
>> 	I would argue that it is better to permit this option to be set
>> at any time, regardless of whether miimon is enabled or not.  This
>> removes an ordering constraint and makes things generally easier to deal
>> with.  Setting the option has no effect if miimon is not set, but that's
>> ok.
>> 
>> 	My comment here would also apply to other options that are
>> "secondary," in the sense that they affect the behavior of other options
>> or modes, e.g., updelay, arp_validate (in another path of this series),
>> arp_ip_targets, etc.
>> 
>> 	I'm aware that this is simply duplicating the existing behavior
>> of the sysfs API, but I'd be in favor of changing that, too.  These
>> limitations are a holdover from the module paramters days, and should
>> have been made more flexible a long time ago.
>
>What I’d like to propose, and I hope that you’ll agree, is we tackle
>this in two phases.  The first phase is to finish the current
>duplication effort to enable netlink equivalents of the attributes in
>sysfs.  This is a fairly mechanical process, preserving existing
>behavior as you point out, and keeps the patches single-minded and easy
>to review.
>
>The second phase is to revisit ordering constraints and find places
>where we can remove constraints or streamline the dependency checks.
>
>Sound like a plan?

	Well, maybe.

	What I want to avoid for the iproute / netlink bonding support
is all of the hoops that the initscripts / sysconfig scripts had to jump
through to obey the current sysfs ordering limtations.

	The primary user of this is going to be iproute.  Will the above
quoted limitations (and their equivalents for various other options)
make any difference with regards to the following:

ip [...] bond arp_validate all arp_interval 1000
ip [...] bond arp_interval 1000 arp_validate all

	I.e., does the ordering matter at the iproute level when
multiple options are specified simultaneously?

	What happens if I do:

ip [...] bond arp_interval 1000 arp_validate all miimon 100

	This is separate from simply changing one thing at a time, e.g.,

ip [...] bond arp_validate all
ip [...] bond arp_interval 1000

	will presuambly hit the test above, and this is where the
ordering stuff comes into play for sure.

	Also, as I look at the iproute patch, it doesn't appear to
accept the text names for the options, only numeric values (e.g., "ip
[...]  bond arp_validate 3").  That appears to be a limitation of Jiri's
original iproute patch as well.  Am I the only person that perfers the
text labels (e.g., arp_validate as "all") to numbers (arp_validate as
"3")?

	That number-only limitation also may make life for initscripts /
sysconfig more difficult, or at least the upgrade path more of a hassle,
since currently the ifcfg-bond* files can have the bonding options
specified as text labels in addition to the numeric values.

	So, honestly, I think if the ordering constraints are going to
be relaxed, it should happen sooner rather than later.  Perhaps not in
the same patch as the netlink support, but ideally at least in the same
series, so there is no real release with the constraints.  Changing the
constraints after the script, etc, conversion is done doesn't really
help much.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com