| Message ID | 5caaac096e5bbbf88ad3aedcc50ff2451f94105c.1562667648.git.aclaudi@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | Fix IPv6 tunnel add when dev param is used | expand |
On 7/9/19 7:16 AM, Andrea Claudi wrote: > This reverts commit ba126dcad20e6d0e472586541d78bdd1ac4f1123. > It breaks tunnel creation when using 'dev' parameter: > > $ ip link add type dummy > $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 > add tunnel "ip6tnl0" failed: File exists > > dev parameter must be used to specify the device to which > the tunnel is binded, and not the tunnel itself. > Stephen: since this reverts a commit in 5.2 it should be in 5.2 as well.
On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote: > > This reverts commit ba126dcad20e6d0e472586541d78bdd1ac4f1123. > It breaks tunnel creation when using 'dev' parameter: > > $ ip link add type dummy > $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 > add tunnel "ip6tnl0" failed: File exists > > dev parameter must be used to specify the device to which > the tunnel is binded, and not the tunnel itself. > > Reported-by: Jianwen Ji <jiji@redhat.com> > Reviewed-by: Matteo Croce <mcroce@redhat.com> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com> > --- > ip/ip6tunnel.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c > index 56fd3466ed062..999408ed801b1 100644 > --- a/ip/ip6tunnel.c > +++ b/ip/ip6tunnel.c > @@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) > p->link = ll_name_to_index(medium); > if (!p->link) > return nodev(medium); > - else > - strlcpy(p->name, medium, sizeof(p->name)); NACK I see that ba126dcad20e6d0e472586541d78bdd1ac4f1123 has broke something but that doesn't mean revert of the original fix is correct way of fixing it. The original fix is fixing the show and change command. Shouldn't you try fixing the add command so that all (show, change, and add) work correctly? > } > return 0; > } > -- > 2.20.1 >
On Tue, 9 Jul 2019 11:14:00 -0600 David Ahern <dsahern@gmail.com> wrote: > On 7/9/19 7:16 AM, Andrea Claudi wrote: > > This reverts commit ba126dcad20e6d0e472586541d78bdd1ac4f1123. > > It breaks tunnel creation when using 'dev' parameter: > > > > $ ip link add type dummy > > $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 > > add tunnel "ip6tnl0" failed: File exists > > > > dev parameter must be used to specify the device to which > > the tunnel is binded, and not the tunnel itself. > > > > Stephen: since this reverts a commit in 5.2 it should be in 5.2 as well. 5.2 has been released. Probably not worth doing 5.2.1 for corner case like this.
On Tue, Jul 9, 2019 at 7:31 PM Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote: > > On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote: > > > > This reverts commit ba126dcad20e6d0e472586541d78bdd1ac4f1123. > > It breaks tunnel creation when using 'dev' parameter: > > > > $ ip link add type dummy > > $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0 > > add tunnel "ip6tnl0" failed: File exists > > > > dev parameter must be used to specify the device to which > > the tunnel is binded, and not the tunnel itself. > > > > Reported-by: Jianwen Ji <jiji@redhat.com> > > Reviewed-by: Matteo Croce <mcroce@redhat.com> > > Signed-off-by: Andrea Claudi <aclaudi@redhat.com> > > --- > > ip/ip6tunnel.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c > > index 56fd3466ed062..999408ed801b1 100644 > > --- a/ip/ip6tunnel.c > > +++ b/ip/ip6tunnel.c > > @@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) > > p->link = ll_name_to_index(medium); > > if (!p->link) > > return nodev(medium); > > - else > > - strlcpy(p->name, medium, sizeof(p->name)); > NACK > > I see that ba126dcad20e6d0e472586541d78bdd1ac4f1123 has broke > something but that doesn't mean revert of the original fix is correct > way of fixing it. The original fix is fixing the show and change > command. Shouldn't you try fixing the add command so that all (show, > change, and add) work correctly? > Hi Mahesh, Thanks for sharing your opinion. I think I already answered this replying to your review of patch 2/2 of this series. To summarize that up I think there is a misunderstanding on the meaning of "dev" param, and "name" param (which is the default) must be used instead in the cases highlighted in your commit. Regards, Andrea > > } > > return 0; > > } > > -- > > 2.20.1 > >
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c index 56fd3466ed062..999408ed801b1 100644 --- a/ip/ip6tunnel.c +++ b/ip/ip6tunnel.c @@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p) p->link = ll_name_to_index(medium); if (!p->link) return nodev(medium); - else - strlcpy(p->name, medium, sizeof(p->name)); } return 0; }