[PATCH] teamd: remove port if adding fails

[Hangbin Liu]

When we add a port to team via teamdctl, some drivers do not support changing
mac address after dev opened, which would lead to the failure of
port_obj_create().

Currently we only destroy the port object if adding port fails. But the port
is still enslaved to team in kernel. The interface shows it's a team_slave
via ip link cmd, but teamdctl state shows nothing. This may make users
confused.

Fix it by removing the port if adding fails.

Reported-by: Vladimir Benes 
Signed-off-by: Hangbin Liu 
---
 teamd/teamd_per_port.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
index 09d1dc7..7f0a45d 100644
--- a/teamd/teamd_per_port.c
+++ b/teamd/teamd_per_port.c
@@ -42,6 +42,8 @@ struct port_obj {
 };

#define _port(port_obj) (&(port_obj)->port)
+static int teamd_port_remove(struct teamd_context *ctx,
+			     struct teamd_port *tdport);

int teamd_port_priv_create_and_get(void **ppriv, struct teamd_port *tdport,
    			   const struct teamd_port_priv *pp,
@@ -203,6 +205,7 @@ static int port_obj_create(struct teamd_context *ctx,
 teamd_event_port_removed:
    teamd_event_port_removed(ctx, tdport);
 list_del:
+	teamd_port_remove(ctx, tdport);
    port_obj_destroy(ctx, port_obj);
    port_obj_free(port_obj);
    return err;

[Jiri Pirko]

Wed, Feb 27, 2019 at 09:05:57AM CET, [e-mail] wrote:
> When we add a port to team via teamdctl, some drivers do not support changing
> mac address after dev opened, which would lead to the failure of
> port_obj_create().

Where exactly this failure happens?
> Currently we only destroy the port object if adding port fails. But the port
> is still enslaved to team in kernel. The interface shows it's a team_slave
> via ip link cmd, but teamdctl state shows nothing. This may make users
> confused.
>
> Fix it by removing the port if adding fails.
>
> Reported-by: Vladimir Benes 
> Signed-off-by: Hangbin Liu
>
> teamd/teamd_per_port.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
> index 09d1dc7..7f0a45d 100644
> --- a/teamd/teamd_per_port.c
> +++ b/teamd/teamd_per_port.c
> @@ -42,6 +42,8 @@ struct port_obj {
> };
>
> #define _port(port_obj) (&(port_obj)->port)
> +static int teamd_port_remove(struct teamd_context *ctx,
> 	     struct teamd_port *tdport);
>
> int teamd_port_priv_create_and_get(void **ppriv, struct teamd_port *tdport,
>    			   const struct teamd_port_priv *pp,
> @@ -203,6 +205,7 @@ static int port_obj_create(struct teamd_context *ctx,
> teamd_event_port_removed:
>    teamd_event_port_removed(ctx, tdport);
> list_del:
> teamd_port_remove(ctx, tdport);

This is not in sync with port_obj_remove() :/
> port_obj_destroy(ctx, port_obj);
>    port_obj_free(port_obj);
>    return err;
> -- 
> 2.19.2

[Hangbin Liu]

On Fri, Mar 08, 2019 at 09:53:49AM +0100, Jiri Pirko wrote:
> Wed, Feb 27, 2019 at 09:05:57AM CET, [e-mail] wrote:
> > When we add a port to team via teamdctl, some drivers do not support changing
> > mac address after dev opened, which would lead to the failure of
> > port_obj_create().
>
> Where exactly this failure happens?

for LACP mode:
port_obj_create()
  - port_priv_init_all()
    - lacp_port_added()
      - lacp_port_set_mac()
> > @@ -203,6 +205,7 @@ static int port_obj_create(struct teamd_context *ctx,
> > teamd_event_port_removed:
> >    teamd_event_port_removed(ctx, tdport);
> > list_del:
> > teamd_port_remove(ctx, tdport);
>
> This is not in sync with port_obj_remove() :/

Oh, I will add this in port_obj_remove()

Thanks
Hangbin

[patchv2 libteam] teamd: remove port if adding fails

[Hangbin Liu]

When we add a port to team via teamdctl, some drivers do not support
changing mac address after dev opened, which would lead to the failure
of port_obj_create(). The call path looks like below for LACP mode:

- port_obj_create()
  - port_priv_init_all()
    - lacp_port_added()
      - lacp_port_set_mac()

Currently, we only destroy the port object if adding port fails. But the
port is still enslaved to team in kernel. IP link command shows the port
is a team_slave, but teamdctl state shows nothing. This may make users
confused.

Fix it by removing the port if adding fails.

v2: also calls teamd_port_remove in port_obj_remove()

Reported-by: Vladimir Benes 
Signed-off-by: Hangbin Liu 
---
 teamd/teamd_per_port.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
index 09d1dc7..f98a90d 100644
--- a/teamd/teamd_per_port.c
+++ b/teamd/teamd_per_port.c
@@ -42,6 +42,8 @@ struct port_obj {
 };

#define _port(port_obj) (&(port_obj)->port)
+static int teamd_port_remove(struct teamd_context *ctx,
+			     struct teamd_port *tdport);

int teamd_port_priv_create_and_get(void **ppriv, struct teamd_port *tdport,
    			   const struct teamd_port_priv *pp,
@@ -203,6 +205,7 @@ static int port_obj_create(struct teamd_context *ctx,
 teamd_event_port_removed:
    teamd_event_port_removed(ctx, tdport);
 list_del:
+	teamd_port_remove(ctx, tdport);
    port_obj_destroy(ctx, port_obj);
    port_obj_free(port_obj);
    return err;
@@ -214,6 +217,7 @@ static void port_obj_remove(struct teamd_context *ctx,
    struct teamd_port *tdport = _port(port_obj);

teamd_event_port_removed(ctx, tdport);
+	teamd_port_remove(ctx, tdport);
    port_obj_destroy(ctx, port_obj);
    port_obj_free(port_obj);
 }

[patchv2 libteam] teamd: remove port if adding fails

[Hangbin Liu]

On Wed, Mar 13, 2019 at 03:04:29PM +0800, Hangbin Liu wrote:
> When we add a port to team via teamdctl, some drivers do not support
> changing mac address after dev opened, which would lead to the failure
> of port_obj_create(). The call path looks like below for LACP mode:
> port_obj_create()
> port_priv_init_all()
> lacp_port_added()
> lacp_port_set_mac()
>
> Currently, we only destroy the port object if adding port fails. But the
> port is still enslaved to team in kernel. IP link command shows the port
> is a team_slave, but teamdctl state shows nothing. This may make users
> confused.
>
> Fix it by removing the port if adding fails.
>
> v2: also calls teamd_port_remove in port_obj_remove()
>
> Reported-by: Vladimir Benes 
> Signed-off-by: Hangbin Liu

Hi Jiri,

Would you please help review this patch?

Thanks
Hangbin

[patchv2 libteam] teamd: remove port if adding fails

[Jiri Pirko]

Wed, Mar 13, 2019 at 08:04:29AM CET, [e-mail] wrote:
> When we add a port to team via teamdctl, some drivers do not support
> changing mac address after dev opened, which would lead to the failure
> of port_obj_create(). The call path looks like below for LACP mode:
> port_obj_create()
> port_priv_init_all()
> lacp_port_added()
> lacp_port_set_mac()
>
> Currently, we only destroy the port object if adding port fails. But the
> port is still enslaved to team in kernel. IP link command shows the port
> is a team_slave, but teamdctl state shows nothing. This may make users
> confused.
>
> Fix it by removing the port if adding fails.
>
> v2: also calls teamd_port_remove in port_obj_remove()
>
> Reported-by: Vladimir Benes 
> Signed-off-by: Hangbin Liu

applied.