[PATCH net-next 1/5] bonding: The fail_over_mac should be set only in ACTIVE_BACKUP mode

[PATCH net-next 1/5] bonding: The fail_over_mac should be set only in ACTIVE_BACKUP mode

[Ding Tianhong]

According the bonding.txt, the option fail_over_mac only affect for
AB mode, but in currect code, the parameter could be set to active
or follow in every mode, this will cause bonding could not set all
slaves of an RR or XOR mode to the same MAC address at enslavement
time, so reset fail_over_mac to 0 if the mode is not ACTIVE_BACKUP.

Fix the wrong variables for pr_err().

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3220b48..ecff04e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4307,12 +4307,14 @@ static int bond_check_params(struct bond_params *params)
 						      fail_over_mac_tbl);
 		if (fail_over_mac_value == -1) {
 			pr_err("Error: invalid fail_over_mac \"%s\"\n",
-			       arp_validate == NULL ? "NULL" : arp_validate);
+			       fail_over_mac == NULL ? "NULL" : fail_over_mac);
 			return -EINVAL;
 		}
 
-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
-			pr_warning("Warning: fail_over_mac only affects active-backup mode.\n");
+		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
+			pr_warning("Warning: fail_over_mac only affects active-backup mode, set it to 0.\n");
+			fail_over_mac_value = BOND_FOM_NONE;
+		}
 	} else {
 		fail_over_mac_value = BOND_FOM_NONE;
 	}
-- 
1.8.0

Re: [PATCH net-next 1/5] bonding: The fail_over_mac should be set only in ACTIVE_BACKUP mode

[Jay Vosburgh]

Ding Tianhong <dingtianhong@huawei.com> wrote:

>According the bonding.txt, the option fail_over_mac only affect for
>AB mode, but in currect code, the parameter could be set to active
>or follow in every mode, this will cause bonding could not set all
>slaves of an RR or XOR mode to the same MAC address at enslavement
>time, so reset fail_over_mac to 0 if the mode is not ACTIVE_BACKUP.

	The correct way to fix this in general is to permit setting an
option at any time, but only have it take effect in active-backup mode.
This minimizes ordering requirements when setting options.

	I would instead modify the bond enslave and removal processing
to check the mode in addition to fail_over_mac when setting a slave's
MAC during enslavement.  The change active slave processing already only
calls the fail_over_mac function when in active-backup mode.  This
should also be a simpler change set.

>Fix the wrong variables for pr_err().
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 3220b48..ecff04e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4307,12 +4307,14 @@ static int bond_check_params(struct bond_params *params)
> 						      fail_over_mac_tbl);
> 		if (fail_over_mac_value == -1) {
> 			pr_err("Error: invalid fail_over_mac \"%s\"\n",
>-			       arp_validate == NULL ? "NULL" : arp_validate);
>+			       fail_over_mac == NULL ? "NULL" : fail_over_mac);

	This part is ok.

> 			return -EINVAL;
> 		}
>
>-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
>-			pr_warning("Warning: fail_over_mac only affects active-backup mode.\n");
>+		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
>+			pr_warning("Warning: fail_over_mac only affects active-backup mode, set it to 0.\n");
>+			fail_over_mac_value = BOND_FOM_NONE;
>+		}

	This part is not.

	I would additionally NAK patches 2, 3, and 4 (noting that 4
inhibits the change to fail_over_mac, but not the warning message that
we're changing it).

	Patch 5 is ok, although it really has nothing to do with
fail_over_mac.

	-J

> 	} else {
> 		fail_over_mac_value = BOND_FOM_NONE;
> 	}
>-- 
>1.8.0

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

Re: [PATCH net-next 1/5] bonding: The fail_over_mac should be set only in ACTIVE_BACKUP mode

[Ding Tianhong]

On 2014/1/22 8:25, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
> 
>> According the bonding.txt, the option fail_over_mac only affect for
>> AB mode, but in currect code, the parameter could be set to active
>> or follow in every mode, this will cause bonding could not set all
>> slaves of an RR or XOR mode to the same MAC address at enslavement
>> time, so reset fail_over_mac to 0 if the mode is not ACTIVE_BACKUP.
> 
> 	The correct way to fix this in general is to permit setting an
> option at any time, but only have it take effect in active-backup mode.
> This minimizes ordering requirements when setting options.
> 
ok

> 	I would instead modify the bond enslave and removal processing
> to check the mode in addition to fail_over_mac when setting a slave's
> MAC during enslavement.  The change active slave processing already only
> calls the fail_over_mac function when in active-backup mode.  This
> should also be a simpler change set.
> 
agree, the current modify actually more redundant.

>> Fix the wrong variables for pr_err().
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 3220b48..ecff04e 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4307,12 +4307,14 @@ static int bond_check_params(struct bond_params *params)
>> 						      fail_over_mac_tbl);
>> 		if (fail_over_mac_value == -1) {
>> 			pr_err("Error: invalid fail_over_mac \"%s\"\n",
>> -			       arp_validate == NULL ? "NULL" : arp_validate);
>> +			       fail_over_mac == NULL ? "NULL" : fail_over_mac);
> 
> 	This part is ok.
> 
>> 			return -EINVAL;
>> 		}
>>
>> -		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
>> -			pr_warning("Warning: fail_over_mac only affects active-backup mode.\n");
>> +		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
>> +			pr_warning("Warning: fail_over_mac only affects active-backup mode, set it to 0.\n");
>> +			fail_over_mac_value = BOND_FOM_NONE;
>> +		}
> 
> 	This part is not.
> 
> 	I would additionally NAK patches 2, 3, and 4 (noting that 4
> inhibits the change to fail_over_mac, but not the warning message that
> we're changing it).
> 
> 	Patch 5 is ok, although it really has nothing to do with
> fail_over_mac.
> 
> 	-J
> 

The whole patchset need to be rebuild, thanks for reviewing.

Regards
Ding

>> 	} else {
>> 		fail_over_mac_value = BOND_FOM_NONE;
>> 	}
>> -- 
>> 1.8.0
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> 
>