[PATCH] Fix remove and add a member port from one team to another.

[Samudrala, Sridhar]

The following scenario where a member port is removed from team1
and added to team2

   teamd -d -c '{"device": "team1", "ports":
{"p4p1": {}}}'
   teamd -d -t team2
   ip link set p4p1 nomaster
   ip link set p4p1 master team2

fails with this error

teamd_team2[17226]: p4p1: Port original hardware address has different length (0) than
team device has (6).

This patch fixes this issue by initializing orig_hw_addr/len of a port
after it is removed from another team so that the correct address is
available when it is added to the new team.

It doesn't fix the case where the member port is directly moved to
a new team without removing it from the original team.

   teamd -d -c '{"device": "team1", "ports":
{"p4p1": {}}}'
   teamd -d -t team2
   ip link set p4p1 master team2

In this scenario teamd gets a notification only after the port is added
to the new team.
It is not clear how to address this scenario OR if we should block
adding a member port to a team if it is already a member of another team.

Signed-off-by: Sridhar Samudrala <sridhar.samudrala(a)intel.com&gt;
---
 libteam/ifinfo.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
index 47b342c..632f5f1 100644
--- a/libteam/ifinfo.c
+++ b/libteam/ifinfo.c
@@ -116,6 +116,11 @@ static void update_hwaddr(struct team_ifinfo *ifinfo, struct
rtnl_link *link)
 			memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
 		set_changed(ifinfo, CHANGED_HWADDR);
 	}
+
+	if (!ifinfo->master_ifindex && !ifinfo->orig_hwaddr_len) {
+		ifinfo->orig_hwaddr_len = hwaddr_len;
+		memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
+	}
 }
 
 static void update_ifname(struct team_ifinfo *ifinfo, struct rtnl_link *link)
-- 
2.1.0

[Jiri Pirko]

Wed, Nov 18, 2015 at 09:29:07PM CET, sridhar.samudrala(a)intel.com wrote:
> The following scenario where a member port is removed from team1
> and added to team2
>
>    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
>    teamd -d -t team2
>    ip link set p4p1 nomaster
>    ip link set p4p1 master team2
>
> fails with this error
>
> teamd_team2[17226]: p4p1: Port original hardware address has different length (0) than
> team device has (6).
>
> This patch fixes this issue by initializing orig_hw_addr/len of a port
> after it is removed from another team so that the correct address is
> available when it is added to the new team.
>
> It doesn't fix the case where the member port is directly moved to
> a new team without removing it from the original team.
>
>    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
>    teamd -d -t team2
>    ip link set p4p1 master team2
>
> In this scenario teamd gets a notification only after the port is added
> to the new team.
> It is not clear how to address this scenario OR if we should block
> adding a member port to a team if it is already a member of another team.
>
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala(a)intel.com&gt;
> ---
>  libteam/ifinfo.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
> index 47b342c..632f5f1 100644
> --- a/libteam/ifinfo.c
> +++ b/libteam/ifinfo.c
> @@ -116,6 +116,11 @@ static void update_hwaddr(struct team_ifinfo *ifinfo, struct
> rtnl_link *link)
>  			memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
>  		set_changed(ifinfo, CHANGED_HWADDR);
>  	}
> +
> +	if (!ifinfo->master_ifindex && !ifinfo->orig_hwaddr_len) {
> +		ifinfo->orig_hwaddr_len = hwaddr_len;
> +		memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
> +	} 
This does not look correct to me.

How the addr len becomes 0?


> }
> 
> static void update_ifname(struct team_ifinfo *ifinfo, struct rtnl_link *link)
>-- 
>2.1.0
>

[Samudrala, Sridhar]

On 11/19/2015 12:37 AM, Jiri Pirko wrote:
>  Wed, Nov 18, 2015 at 09:29:07PM CET, sridhar.samudrala(a)intel.com
> wrote:
> > The following scenario where a member port is removed from team1
> > and added to team2
> >
> >    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
> >    teamd -d -t team2
> >    ip link set p4p1 nomaster
> >    ip link set p4p1 master team2
> >
> > fails with this error
> >
> > teamd_team2[17226]: p4p1: Port original hardware address has different length (0)
> than team device has (6).
> >
> > This patch fixes this issue by initializing orig_hw_addr/len of a port
> > after it is removed from another team so that the correct address is
> > available when it is added to the new team.
> >
> > It doesn't fix the case where the member port is directly moved to
> > a new team without removing it from the original team.
> >
> >    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
> >    teamd -d -t team2
> >    ip link set p4p1 master team2
> >
> > In this scenario teamd gets a notification only after the port is added
> > to the new team.
> > It is not clear how to address this scenario OR if we should block
> > adding a member port to a team if it is already a member of another team.
> >
> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala(a)intel.com&gt;
> > ---
> > libteam/ifinfo.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
> > index 47b342c..632f5f1 100644
> > --- a/libteam/ifinfo.c
> > +++ b/libteam/ifinfo.c
> > @@ -116,6 +116,11 @@ static void update_hwaddr(struct team_ifinfo *ifinfo, struct
> rtnl_link *link)
> > 			memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
> > 		set_changed(ifinfo, CHANGED_HWADDR);
> > 	}
> > +
> > +	if (!ifinfo->master_ifindex && !ifinfo->orig_hwaddr_len) {
> > +		ifinfo->orig_hwaddr_len = hwaddr_len;
> > +		memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
> > +	}
>  This does not look correct to me.
>
>  How the addr len becomes 0? 
It doesn't become 0. It is not initialized in this
scenario as the port 
is a member of another team and master_ifindex points to that team.
 From update_hwaddr()
         if (ifinfo->hwaddr_len != hwaddr_len) {
                 ifinfo->hwaddr_len = hwaddr_len;
                 if (!ifinfo->master_ifindex)
                         ifinfo->orig_hwaddr_len = hwaddr_len;
                 set_changed(ifinfo, CHANGED_HWADDR_LEN);
         }


> > }
> >
> > static void update_ifname(struct team_ifinfo *ifinfo, struct rtnl_link *link)
> > -- 
> > 2.1.0
> >

[Jiri Pirko]

Wed, Nov 18, 2015 at 09:29:07PM CET, sridhar.samudrala(a)intel.com wrote:
> The following scenario where a member port is removed from team1
> and added to team2
>
>    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
>    teamd -d -t team2
>    ip link set p4p1 nomaster
>    ip link set p4p1 master team2
>
> fails with this error
>
> teamd_team2[17226]: p4p1: Port original hardware address has different length (0) than
> team device has (6).
>
> This patch fixes this issue by initializing orig_hw_addr/len of a port
> after it is removed from another team so that the correct address is
> available when it is added to the new team.
>
> It doesn't fix the case where the member port is directly moved to
> a new team without removing it from the original team.
>
>    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
>    teamd -d -t team2
>    ip link set p4p1 master team2
>
> In this scenario teamd gets a notification only after the port is added
> to the new team.
> It is not clear how to address this scenario OR if we should block
> adding a member port to a team if it is already a member of another team. 
I believe that we should not block this. Does not make sense.

Looks like we are probably missing some event. Have to check this.

Jiri

>
>Signed-off-by: Sridhar Samudrala <sridhar.samudrala(a)intel.com&gt;
>---
> libteam/ifinfo.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
>index 47b342c..632f5f1 100644
>--- a/libteam/ifinfo.c
>+++ b/libteam/ifinfo.c
>@@ -116,6 +116,11 @@ static void update_hwaddr(struct team_ifinfo *ifinfo, struct
rtnl_link *link)
> 			memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
> 		set_changed(ifinfo, CHANGED_HWADDR);
> 	}
>+
>+	if (!ifinfo->master_ifindex && !ifinfo->orig_hwaddr_len) {
>+		ifinfo->orig_hwaddr_len = hwaddr_len;
>+		memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
>+	}
> }
> 
> static void update_ifname(struct team_ifinfo *ifinfo, struct rtnl_link *link)
>-- 
>2.1.0
>

[Jiri Pirko]

Wed, Nov 18, 2015 at 09:29:07PM CET, sridhar.samudrala(a)intel.com wrote:
> The following scenario where a member port is removed from team1
> and added to team2
>
>    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
>    teamd -d -t team2
>    ip link set p4p1 nomaster
>    ip link set p4p1 master team2
>
> fails with this error
>
> teamd_team2[17226]: p4p1: Port original hardware address has different length (0) than
> team device has (6).
>
> This patch fixes this issue by initializing orig_hw_addr/len of a port
> after it is removed from another team so that the correct address is
> available when it is added to the new team.
>
> It doesn't fix the case where the member port is directly moved to
> a new team without removing it from the original team.
>
>    teamd -d -c '{"device": "team1", "ports":
> {"p4p1": {}}}'
>    teamd -d -t team2
>    ip link set p4p1 master team2
>
> In this scenario teamd gets a notification only after the port is added
> to the new team.
> It is not clear how to address this scenario OR if we should block
> adding a member port to a team if it is already a member of another team. 
I was not able to reproduce the case 1, but I see the problem. Kernel
module stores original addr during port enslave. I think that the best
way would be just pass this stored original address from kernel in port
list/event. I'll implement this in kernel and libteam.

Thanks.


>
>Signed-off-by: Sridhar Samudrala <sridhar.samudrala(a)intel.com&gt;
>---
> libteam/ifinfo.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
>index 47b342c..632f5f1 100644
>--- a/libteam/ifinfo.c
>+++ b/libteam/ifinfo.c
>@@ -116,6 +116,11 @@ static void update_hwaddr(struct team_ifinfo *ifinfo, struct
rtnl_link *link)
> 			memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
> 		set_changed(ifinfo, CHANGED_HWADDR);
> 	}
>+
>+	if (!ifinfo->master_ifindex && !ifinfo->orig_hwaddr_len) {
>+		ifinfo->orig_hwaddr_len = hwaddr_len;
>+		memcpy(ifinfo->orig_hwaddr, hwaddr, hwaddr_len);
>+	}
> }
> 
> static void update_ifname(struct team_ifinfo *ifinfo, struct rtnl_link *link)
>-- 
>2.1.0
>