bonding: update the primary when slave name changed

PATCH In review Ding Tianhong 2 participants 9 messages Jan 9, 2014 – Jan 10, 2014

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Ding Tianhong

author

2014-01-09 11:21 UTC #0 lore raw

PATCH 1/4 bonding: update the primary when slave name changed

If the primary_slave's name changed, but the bond->prams.primay was still using the old name, it is conflict with the meaning of the primary, so update the primary when the slave change its name.

Signed-off-by Ding Tianhong <[email protected]>

---
 drivers/net/bonding/bond_main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +12 -2

index e06c445..de646e2 100644
@@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		break;
 	case NETDEV_CHANGENAME:
-		/*
-		 * TODO: handle changing the primary's name
+		/* if the primary's name changed,
+		 * save the new name for primary.
 		 */
+		if (USES_PRIMARY(bond->params.mode) &&
+		    bond->params.primary[0]) {
+			if (bond->primary_slave &&
+			    strcmp(bond->params.primary,
+				   bond->primary_slave->dev->name)) {
+				strncpy(bond->params.primary,
+					bond->primary_slave->dev->name,
+					IFNAMSIZ);
+			}
+		}
 		break;
 	case NETDEV_FEAT_CHANGE:
 		bond_compute_features(bond);

Signature · 1 line

1.8.0

Veaceslav Falico

2014-01-09 11:49 UTC #1 lore raw

Quoted 3 lines · Ding Tianhong

If the primary_slave's name changed, but the bond->prams.primay was still using the old name, it is conflict with the meaning of the primary, so update the primary when the slave change its name.

Nope, the bonding parameter, which is set by the user, shouldn't change because of an interface name change.

Quoted 36 lines

Signed-off-by Ding Tianhong <[email protected]>

---
drivers/net/bonding/bond_main.c | 14 ++++++++++++--

1 file changed, 12 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +12 -2

index e06c445..de646e2 100644
@@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		break;
 	case NETDEV_CHANGENAME:
-		/*
-		 * TODO: handle changing the primary's name
+		/* if the primary's name changed,
+		 * save the new name for primary.
 		 */
+		if (USES_PRIMARY(bond->params.mode) &&
+		    bond->params.primary[0]) {
+			if (bond->primary_slave &&
+			    strcmp(bond->params.primary,
+				   bond->primary_slave->dev->name)) {
+				strncpy(bond->params.primary,
+					bond->primary_slave->dev->name,
+					IFNAMSIZ);
+			}
+		}
 		break;
 	case NETDEV_FEAT_CHANGE:
 		bond_compute_features(bond);

Signature · 1 line

1.8.0

Ding Tianhong

author

2014-01-09 12:27 UTC #2 lore raw

Quoted 8 lines · Veaceslav Falico

Quoted 3 lines · Ding Tianhong
If the primary_slave's name changed, but the bond->prams.primay was still using the old name, it is conflict with the meaning of the primary, so update the primary when the slave change its name.
Nope, the bonding parameter, which is set by the user, shouldn't change because of an interface name change.

Yes, I know what you mean, but it is not bug fix, just make it more better, do not you feel it strange that the primary was different with primary_slave's name?

Regards
Ding

Quoted 38 lines

Quoted 36 lines

Signed-off-by Ding Tianhong <[email protected]>

---
drivers/net/bonding/bond_main.c | 14 ++++++++++++--

1 file changed, 12 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +12 -2

index e06c445..de646e2 100644
@@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
         */
        break;
    case NETDEV_CHANGENAME:
-        /*
-         * TODO: handle changing the primary's name
+        /* if the primary's name changed,
+         * save the new name for primary.
         */
+        if (USES_PRIMARY(bond->params.mode) &&
+            bond->params.primary[0]) {
+            if (bond->primary_slave &&
+                strcmp(bond->params.primary,
+                   bond->primary_slave->dev->name)) {
+                strncpy(bond->params.primary,
+                    bond->primary_slave->dev->name,
+                    IFNAMSIZ);
+            }
+        }
        break;
    case NETDEV_FEAT_CHANGE:
        bond_compute_features(bond);

Signature · 1 line

1.8.0

Ding Tianhong

author

2014-01-10 04:21 UTC #4 lore raw

Quoted 10 lines · Veaceslav Falico

Quoted 9 lines · Ding Tianhong
Quoted 8 lines · Veaceslav Falico
Quoted 3 lines · Ding Tianhong
If the primary_slave's name changed, but the bond->prams.primay was still using the old name, it is conflict with the meaning of the primary, so update the primary when the slave change its name.
Nope, the bonding parameter, which is set by the user, shouldn't change because of an interface name change.

Quoted 7 lines

Quoted 2 lines
Yes, I know what you mean, but it is not bug fix, just make it more better, do not you feel it strange that the primary was different with primary_slave's name?
Yep, that's an issue - that's why there is the TODO. We shouldn't, though, change the primary param, but rather check if the slave (that changed name) is (already not) eligible for primary_slave.

Ok，So，summarize your and my opinion, I think there are two ways to fix this:

1. just like my patch said.
2. check if the primary is not the primary_slave, make the primary_slave = NULL, this means the primary_slave is no valid.
3. ?? did you have any better ideas?

Regards
Ding

Quoted 48 lines

Quoted 45 lines

Regards
Ding

Quoted 38 lines

Quoted 36 lines

Signed-off-by Ding Tianhong <[email protected]>

---
drivers/net/bonding/bond_main.c | 14 ++++++++++++--

1 file changed, 12 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +12 -2

index e06c445..de646e2 100644
@@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
         */
        break;
    case NETDEV_CHANGENAME:
-        /*
-         * TODO: handle changing the primary's name
+        /* if the primary's name changed,
+         * save the new name for primary.
         */
+        if (USES_PRIMARY(bond->params.mode) &&
+            bond->params.primary[0]) {
+            if (bond->primary_slave &&
+                strcmp(bond->params.primary,
+                   bond->primary_slave->dev->name)) {
+                strncpy(bond->params.primary,
+                    bond->primary_slave->dev->name,
+                    IFNAMSIZ);
+            }
+        }
        break;
    case NETDEV_FEAT_CHANGE:
        bond_compute_features(bond);

Signature · 1 line

1.8.0

Veaceslav Falico

2014-01-10 07:47 UTC #5 lore raw

Quoted 14 lines · Ding Tianhong

Quoted 13 lines · Veaceslav Falico
Quoted 11 lines · Ding Tianhong
Quoted 8 lines · Veaceslav Falico
Quoted 3 lines · Ding Tianhong
If the primary_slave's name changed, but the bond->prams.primay was still using the old name, it is conflict with the meaning of the primary, so update the primary when the slave change its name.
Nope, the bonding parameter, which is set by the user, shouldn't change because of an interface name change.
Yes, I know what you mean, but it is not bug fix, just make it more better, do not you feel it strange that the primary was different with primary_slave's name?

Quoted 8 lines

Quoted 4 lines
Yep, that's an issue - that's why there is the TODO. We shouldn't, though, change the primary param, but rather check if the slave (that changed name) is (already not) eligible for primary_slave.
Ok，So，summarize your and my opinion, I think there are two ways to fix this:
1. just like my patch said.

No, the primary string is user-set, and it should *not* be changed by kernel.

Quoted 2 lines

2. check if the primary is not the primary_slave, make the primary_slave = NULL, this means the primary_slave is no valid.

Check the slave that changed name - if it's the primary slave, remove it, and see if we need to select the new active slave. If it's not the primray slave, and we don't have one - select it as a new primary and, again, see if we need to select a new active slave.

Quoted 55 lines

3. ?? did you have any better ideas?

Regards
Ding

Quoted 48 lines

Quoted 45 lines

Regards
Ding

Quoted 38 lines

Quoted 36 lines

Signed-off-by Ding Tianhong <[email protected]>

---
drivers/net/bonding/bond_main.c | 14 ++++++++++++--

1 file changed, 12 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_main.c +12 -3

index e06c445..de646e2 100644
@@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
         */
        break;
    case NETDEV_CHANGENAME:
-        /*
-         * TODO: handle changing the primary's name
+        /* if the primary's name changed,
+         * save the new name for primary.
         */
+        if (USES_PRIMARY(bond->params.mode) &&
+            bond->params.primary[0]) {
+            if (bond->primary_slave &&
+                strcmp(bond->params.primary,
+                   bond->primary_slave->dev->name)) {
+                strncpy(bond->params.primary,
+                    bond->primary_slave->dev->name,
+                    IFNAMSIZ);
+            }
+        }
        break;
    case NETDEV_FEAT_CHANGE:
        bond_compute_features(bond);
--

1.8.0

Ding Tianhong

author

2014-01-10 11:05 UTC #6 lore raw

Quoted 24 lines · Veaceslav Falico

Quoted 14 lines · Ding Tianhong
Quoted 13 lines · Veaceslav Falico
Quoted 11 lines · Ding Tianhong
Quoted 8 lines · Veaceslav Falico
Quoted 3 lines · Ding Tianhong
If the primary_slave's name changed, but the bond->prams.primay was still using the old name, it is conflict with the meaning of the primary, so update the primary when the slave change its name.
Nope, the bonding parameter, which is set by the user, shouldn't change because of an interface name change.
Yes, I know what you mean, but it is not bug fix, just make it more better, do not you feel it strange that the primary was different with primary_slave's name?
Quoted 8 lines
Quoted 4 lines
Yep, that's an issue - that's why there is the TODO. We shouldn't, though, change the primary param, but rather check if the slave (that changed name) is (already not) eligible for primary_slave.
Ok，So，summarize your and my opinion, I think there are two ways to fix this:
1. just like my patch said.

Quoted 8 lines

No, the primary string is user-set, and it should *not* be changed by kernel.
Quoted 2 lines
2. check if the primary is not the primary_slave, make the primary_slave = NULL, this means the primary_slave is no valid.
Check the slave that changed name - if it's the primary slave, remove it, and see if we need to select the new active slave.

Ok, agree.

Quoted 3 lines

If it's not the primray
slave, and we don't have one - select it as a new primary and, again, see if we need to select a new active slave.

I don't think so , I think if it is not the primary slave and we don't have one, no need to do anything, just a normal slave change its name.

Regards
Ding

Quoted 16 lines

Quoted 13 lines
3. ?? did you have any better ideas?
Regards
Ding
Quoted 4 lines
Quoted 1 line
.
.

Veaceslav Falico

2014-01-10 11:14 UTC #7 lore raw

Quoted 7 lines · Ding Tianhong

Quoted 3 lines · Veaceslav Falico
If it's not the primray
slave, and we don't have one - select it as a new primary and, again, see if we need to select a new active slave.
I don't think so , I think if it is not the primary slave and we don't have one, no need to do anything, just a normal slave change its name.

If primary == "my_eth0", you have 2 slaves - "eth0" and "eth1", thus null primary_slave, and rename eth0 to my_eth0 - then you need to set primary_slave to my_eth0.

If primary == "my_eth0", you have 2 slaves - "my_eth0" and "eth1", thus primary_slave == dev with name "my_eth0", and rename "my_eth0" to "eth0" - then you must set primary_slave to NULL.

And after either of these you must see if you need to re-select the active slave, as it might have been forced by the primary_slave, which has been modified.

You might also want to add some pr_info() about adding/removing primary_slave, as the user to be aware.

Ding Tianhong

author

2014-01-10 11:56 UTC #8 lore raw

Quoted 17 lines · Veaceslav Falico

Quoted 7 lines · Ding Tianhong
Quoted 3 lines · Veaceslav Falico
If it's not the primray
slave, and we don't have one - select it as a new primary and, again, see if we need to select a new active slave.
I don't think so , I think if it is not the primary slave and we don't have one, no need to do anything, just a normal slave change its name.
If primary == "my_eth0", you have 2 slaves - "eth0" and "eth1", thus null primary_slave, and rename eth0 to my_eth0 - then you need to set primary_slave to my_eth0.
If primary == "my_eth0", you have 2 slaves - "my_eth0" and "eth1", thus primary_slave == dev with name "my_eth0", and rename "my_eth0" to "eth0" - then you must set primary_slave to NULL.

Quoted 7 lines

And after either of these you must see if you need to re-select the active slave, as it might have been forced by the primary_slave, which has been modified.
You might also want to add some pr_info() about adding/removing primary_slave, as the user to be aware.

Ok thanks.

Regards
Ding

Quoted 1 line