Re: [PATCH] bonding: document two undocumented options.

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

Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:

>Le 03/08/2011 12:44, David Miller a écrit :
>> From: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>> Date: Tue,  2 Aug 2011 22:06:55 +0200
>>
>>> Commit 655f8919d549ad1872e24d826b6ce42530516d2e
>>>      bonding: add min links parameter to 802.3ad
>>>
>>> and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f
>>>      bonding: add all_slaves_active parameter
>>>
>>> introduced new options to bonding, but didn't provide the documentation
>>> for those options.
>>>
>>> Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>>
>> Please explicitly mention in each new entry what the default
>> setting is.
>
>Unfortunately, I failed to find a place in the bonding code where the
>max_links option is initialized with a default value. So I must assume
>default value is zero which should cause carrier to always be asserted, or
>undefined, which should cause interesting side effects...
>
>The obvious default value should be 1, but I cannot confirm it is.

	Looking at it now, I see no initialization, and it's a static,
so I believe it will end up being zero.  From the code, zero seems like
the proper default, since it will make this test never pass:

                /* are enough slaves available to consider link up? */
                if (active->num_of_ports < bond->params.min_links) {
                        if (netif_carrier_ok(bond->dev)) {
                                netif_carrier_off(bond->dev);
                                return 1;
                        }

	This will cause carrier to be asserted (for 802.3ad mode)
whenever there is an active aggregator, regardless of the number of
available links in that aggregator.

>Stephen, as the author of this feature, can you please clarify what the default value for min_links is?
>
>V2 will follow, giving the real default value for all_slaves_active and
>what I consider the sensible default value for max_links, even if the
>technical real default value is currently unclear.

	I think the actual and sensible default are both zero, although
the documentation should probably mention that a value of zero is magic
and won't ever set the bond down due to too few ports (links) active.

	Or, perhaps describe it how it actually works: if there are
fewer than "min_links" ports in the active aggregator, the bond is set
carrier down.  The default min_links value of zero means that the bond
will never be set down due to having too few ports active.

	-J

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