4:15 p.m.
I just checked the compilation without CONFIG_IPV6 and fixed what GCC was
complaining about. Nothing fancy.
Matthieu Baerts (3):
mptcp: fix compilation warning without IPV6
mptcp: fix compilation error without IPV6
mptcp:basic: check for v6 family
net/mptcp/basic.c | 2 +-
net/mptcp/options.c | 2 ++
net/mptcp/pm.c | 4 +++-
3 files changed, 6 insertions(+), 2 deletions(-)
--
2.20.1
4:15 p.m.
New subject: [PATCH 1/3] mptcp: fix compilation warning without IPV6
net/mptcp/pm.c:228:23: warning: unused variable ‘addr6’ [-Wunused-variable]
228 | struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)saddr;
| ^~~~~
Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
---
Notes:
To squash in "mptcp: Add path manager interface"
net/mptcp/pm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index b14308ad11a3..f521bf0c5f69 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -224,8 +224,10 @@ void mptcp_pm_add_addr6(struct mptcp_sock *msk, const struct in6_addr
*addr,
int mptcp_pm_addr_signal(struct mptcp_sock *msk, u8 *id,
struct sockaddr_storage *saddr)
{
- struct sockaddr_in *addr = (struct sockaddr_in *)saddr;
+#if IS_ENABLED(CONFIG_IPV6)
struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)saddr;
+#endif
+ struct sockaddr_in *addr = (struct sockaddr_in *)saddr;
if (!msk->pm.local_valid)
return -1;
--
2.20.1
4:17 p.m.
New subject: [PATCH 1/3] mptcp: fix compilation warning without IPV6
Hi,
On 16/10/2019 18:15, Matthieu Baerts wrote:
net/mptcp/pm.c:228:23: warning: unused variable ‘addr6’
[-Wunused-variable]
228 | struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)saddr;
| ^~~~~
Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
---
Notes:
To squash in "mptcp: Add path manager interface"
net/mptcp/pm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index b14308ad11a3..f521bf0c5f69 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -224,8 +224,10 @@ void mptcp_pm_add_addr6(struct mptcp_sock *msk, const struct
in6_addr *addr,
int mptcp_pm_addr_signal(struct mptcp_sock *msk, u8 *id,
struct sockaddr_storage *saddr)
{
- struct sockaddr_in *addr = (struct sockaddr_in *)saddr;
+#if IS_ENABLED(CONFIG_IPV6)
struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)saddr;
+#endif
+ struct sockaddr_in *addr = (struct sockaddr_in *)saddr;
I forgot to say that I also fixed the "reversed Christmas tree" order.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
4:15 p.m.
New subject: [PATCH 2/3] mptcp: fix compilation error without IPV6
net/mptcp/options.c: In function ‘mptcp_established_options_addr’:
net/mptcp/options.c:478:9: error: ‘struct mptcp_out_options’ has no member named
‘addr6’; did you mean ‘addr’?
478 | opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
| ^~~~~
| addr
Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
---
Notes:
To squash in "mptcp: Add ADD_ADDR handling"
net/mptcp/options.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c948951509e1..6cfcf2ce44fe 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -470,6 +470,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
opts->addr_id = id;
opts->addr = ((struct sockaddr_in *)&saddr)->sin_addr;
*size = TCPOLEN_MPTCP_ADD_ADDR;
+#if IS_ENABLED(CONFIG_IPV6)
} else if (saddr.ss_family == AF_INET6) {
if (remaining < TCPOLEN_MPTCP_ADD_ADDR6)
return false;
@@ -477,6 +478,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
opts->addr_id = id;
opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
*size = TCPOLEN_MPTCP_ADD_ADDR6;
+#endif
}
msk->addr_signal = 0;
--
2.20.1
2:18 p.m.
New subject: [PATCH 2/3] mptcp: fix compilation error without IPV6
On Wed, 2019-10-16 at 18:15 +0200, Matthieu Baerts wrote:
net/mptcp/options.c: In function ‘mptcp_established_options_addr’:
net/mptcp/options.c:478:9: error: ‘struct mptcp_out_options’ has no member named
‘addr6’; did you mean ‘addr’?
478 | opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
| ^~~~~
| addr
Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
---
Notes:
To squash in "mptcp: Add ADD_ADDR handling"
net/mptcp/options.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c948951509e1..6cfcf2ce44fe 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -470,6 +470,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
opts->addr_id = id;
opts->addr = ((struct sockaddr_in *)&saddr)->sin_addr;
*size = TCPOLEN_MPTCP_ADD_ADDR;
+#if IS_ENABLED(CONFIG_IPV6)
} else if (saddr.ss_family == AF_INET6) {
if (remaining < TCPOLEN_MPTCP_ADD_ADDR6)
return false;
@@ -477,6 +478,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
opts->addr_id = id;
opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
*size = TCPOLEN_MPTCP_ADD_ADDR6;
+#endif
I think that the preferred codying style here is something alike:
// ipv4 branch
}
#if IS_ENABLED(CONFIG_IPV6)
else if (/* ... */) {
// ...
}
#endif
yep, without the preprocess conditional that would be against the
codying rules, but looks like checkpatch would be happy here.
The rationale is that is more clear in both the patch and the resulting
code what is protected by the conditional.
/P
2:22 p.m.
New subject: [PATCH 2/3] mptcp: fix compilation error without IPV6
Hi Paolo,
Thank you for the review!
On 17/10/2019 16:18, Paolo Abeni wrote:
On Wed, 2019-10-16 at 18:15 +0200, Matthieu Baerts wrote:
> net/mptcp/options.c: In function ‘mptcp_established_options_addr’:
> net/mptcp/options.c:478:9: error: ‘struct mptcp_out_options’ has no member named
‘addr6’; did you mean ‘addr’?
> 478 | opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
> | ^~~~~
> | addr
>
> Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> ---
>
> Notes:
> To squash in "mptcp: Add ADD_ADDR handling"
>
> net/mptcp/options.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index c948951509e1..6cfcf2ce44fe 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -470,6 +470,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
> opts->addr_id = id;
> opts->addr = ((struct sockaddr_in *)&saddr)->sin_addr;
> *size = TCPOLEN_MPTCP_ADD_ADDR;
> +#if IS_ENABLED(CONFIG_IPV6)
> } else if (saddr.ss_family == AF_INET6) {
> if (remaining < TCPOLEN_MPTCP_ADD_ADDR6)
> return false;
> @@ -477,6 +478,7 @@ static bool mptcp_established_options_addr(struct sock *sk,
> opts->addr_id = id;
> opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
> *size = TCPOLEN_MPTCP_ADD_ADDR6;
> +#endif
I think that the preferred codying style here is something alike:
// ipv4 branch
}
#if IS_ENABLED(CONFIG_IPV6)
else if (/* ... */) {
// ...
}
#endif
yep, without the preprocess conditional that would be against the
codying rules, but looks like checkpatch would be happy here.
The rationale is that is more clear in both the patch and the resulting
code what is protected by the conditional.
Fine for me! I copied what was done in net/mptcp/basic.c:75
Do you want me to apply the same change there as well?
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
2:32 p.m.
New subject: [PATCH 2/3] mptcp: fix compilation error without IPV6
On Thu, 2019-10-17 at 16:22 +0200, Matthieu Baerts wrote:
Hi Paolo,
Thank you for the review!
On 17/10/2019 16:18, Paolo Abeni wrote:
> On Wed, 2019-10-16 at 18:15 +0200, Matthieu Baerts wrote:
> > net/mptcp/options.c: In function ‘mptcp_established_options_addr’:
> > net/mptcp/options.c:478:9: error: ‘struct mptcp_out_options’ has no member
named ‘addr6’; did you mean ‘addr’?
> > 478 | opts->addr6 = ((struct sockaddr_in6
*)&saddr)->sin6_addr;
> > | ^~~~~
> > | addr
> >
> > Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> > ---
> >
> > Notes:
> > To squash in "mptcp: Add ADD_ADDR handling"
> >
> > net/mptcp/options.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index c948951509e1..6cfcf2ce44fe 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -470,6 +470,7 @@ static bool mptcp_established_options_addr(struct sock
*sk,
> > opts->addr_id = id;
> > opts->addr = ((struct sockaddr_in *)&saddr)->sin_addr;
> > *size = TCPOLEN_MPTCP_ADD_ADDR;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > } else if (saddr.ss_family == AF_INET6) {
> > if (remaining < TCPOLEN_MPTCP_ADD_ADDR6)
> > return false;
> > @@ -477,6 +478,7 @@ static bool mptcp_established_options_addr(struct sock
*sk,
> > opts->addr_id = id;
> > opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
> > *size = TCPOLEN_MPTCP_ADD_ADDR6;
> > +#endif
>
> I think that the preferred codying style here is something alike:
>
> // ipv4 branch
> }
> #if IS_ENABLED(CONFIG_IPV6)
> else if (/* ... */) {
> // ...
> }
> #endif
>
> yep, without the preprocess conditional that would be against the
> codying rules, but looks like checkpatch would be happy here.
>
> The rationale is that is more clear in both the patch and the resulting
> code what is protected by the conditional.
Fine for me! I copied what was done in net/mptcp/basic.c:75
Do you want me to apply the same change there as well?
Oops... I missed that - and net/mptcp/options.c:587, too.
I personally would go with what I mentioned above. Since there is a
build to fix, I think we can merge as is for speed's sake, and update
all the places later.
Cheers,
Paolo
2:43 p.m.
New subject: [PATCH 2/3] mptcp: fix compilation error without IPV6
Hi Paolo,
On 17/10/2019 16:32, Paolo Abeni wrote:
On Thu, 2019-10-17 at 16:22 +0200, Matthieu Baerts wrote:
> Hi Paolo,
>
> Thank you for the review!
>
> On 17/10/2019 16:18, Paolo Abeni wrote:
>> On Wed, 2019-10-16 at 18:15 +0200, Matthieu Baerts wrote:
>>> net/mptcp/options.c: In function ‘mptcp_established_options_addr’:
>>> net/mptcp/options.c:478:9: error: ‘struct mptcp_out_options’ has no
member named ‘addr6’; did you mean ‘addr’?
>>> 478 | opts->addr6 = ((struct sockaddr_in6
*)&saddr)->sin6_addr;
>>> | ^~~~~
>>> | addr
>>>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
>>> ---
>>>
>>> Notes:
>>> To squash in "mptcp: Add ADD_ADDR handling"
>>>
>>> net/mptcp/options.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index c948951509e1..6cfcf2ce44fe 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -470,6 +470,7 @@ static bool mptcp_established_options_addr(struct sock
*sk,
>>> opts->addr_id = id;
>>> opts->addr = ((struct sockaddr_in *)&saddr)->sin_addr;
>>> *size = TCPOLEN_MPTCP_ADD_ADDR;
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> } else if (saddr.ss_family == AF_INET6) {
>>> if (remaining < TCPOLEN_MPTCP_ADD_ADDR6)
>>> return false;
>>> @@ -477,6 +478,7 @@ static bool mptcp_established_options_addr(struct sock
*sk,
>>> opts->addr_id = id;
>>> opts->addr6 = ((struct sockaddr_in6 *)&saddr)->sin6_addr;
>>> *size = TCPOLEN_MPTCP_ADD_ADDR6;
>>> +#endif
>>
>> I think that the preferred codying style here is something alike:
>>
>> // ipv4 branch
>> }
>> #if IS_ENABLED(CONFIG_IPV6)
>> else if (/* ... */) {
>> // ...
>> }
>> #endif
>>
>> yep, without the preprocess conditional that would be against the
>> codying rules, but looks like checkpatch would be happy here.
>>
>> The rationale is that is more clear in both the patch and the resulting
>> code what is protected by the conditional.
>
> Fine for me! I copied what was done in net/mptcp/basic.c:75
> Do you want me to apply the same change there as well?
Oops... I missed that - and net/mptcp/options.c:587, too.
Which code are you referring to?
https://github.com/multipath-tcp/mptcp_net-next/blob/export/net/mptcp/opt...
I personally would go with what I mentioned above. Since there is a
build to fix, I think we can merge as is for speed's sake, and update
all the places later.
I was already updating the patches. If you don't mind, I am going to
send them and apply the ones that fix the build.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
4:15 p.m.
New subject: [PATCH 3/3] mptcp:basic: check for v6 family
Instead of by default taking IPv6 if it is not v4.
I don't know exactly what are the recommendations about that but it
seems we can often see code checking 'if v4' then 'else if v6', not
checking 'if v4' then 'else it is v6'.
Anyway, that's a detail, more to know what we should do next time in
this situation :)
Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
---
Notes:
To squash in "mptcp: Implement basic path manager"
net/mptcp/basic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/basic.c b/net/mptcp/basic.c
index 5520aae3a6e7..4710607a3fc2 100644
--- a/net/mptcp/basic.c
+++ b/net/mptcp/basic.c
@@ -218,7 +218,7 @@ static void create_subflow_worker(struct work_struct *work)
else if (pm->local_family == AF_INET)
mptcp_pm_create_subflow(pm->token, pm->remote_id, NULL);
#if IS_ENABLED(CONFIG_IPV6)
- else
+ else if (pm->local_family == AF_INET6)
mptcp_pm_create_subflow6(pm->token, pm->remote_id, NULL);
#endif
--
2.20.1
4:28 p.m.
New subject: [PATCH 3/3] mptcp:basic: check for v6 family
On Wed, 2019-10-16 at 18:15 +0200, Matthieu Baerts wrote:
Instead of by default taking IPv6 if it is not v4.
I don't know exactly what are the recommendations about that but it
seems we can often see code checking 'if v4' then 'else if v6', not
checking 'if v4' then 'else it is v6'.
In this situation I think it is OK to use 'else it is v6' because this code is
within the module and we know the field is set correctly (i.e. if local_valid
is set then local_family is set to one or the other). That has been my
approach: use "else if v6" when the value comes from elsewhere. But YMMV.
Peter.
Anyway, that's a detail, more to know what we should do next time
in
this situation :)
Signed-off-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
---
Notes:
To squash in "mptcp: Implement basic path manager"
net/mptcp/basic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/basic.c b/net/mptcp/basic.c
index 5520aae3a6e7..4710607a3fc2 100644
--- a/net/mptcp/basic.c
+++ b/net/mptcp/basic.c
@@ -218,7 +218,7 @@ static void create_subflow_worker(struct work_struct *work)
else if (pm->local_family == AF_INET)
mptcp_pm_create_subflow(pm->token, pm->remote_id, NULL);
#if IS_ENABLED(CONFIG_IPV6)
- else
+ else if (pm->local_family == AF_INET6)
mptcp_pm_create_subflow6(pm->token, pm->remote_id, NULL);
#endif
4:44 p.m.
New subject: [PATCH 3/3] mptcp:basic: check for v6 family
Hi Peter,
On 16/10/2019 18:28, Peter Krystad wrote:
On Wed, 2019-10-16 at 18:15 +0200, Matthieu Baerts wrote:
> Instead of by default taking IPv6 if it is not v4.
>
> I don't know exactly what are the recommendations about that but it
> seems we can often see code checking 'if v4' then 'else if v6', not
> checking 'if v4' then 'else it is v6'.
In this situation I think it is OK to use 'else it is v6' because this code is
within the module and we know the field is set correctly (i.e. if local_valid
is set then local_family is set to one or the other). That has been my
approach: use "else if v6" when the value comes from elsewhere. But YMMV.
Thank you for this comment.
Sounds good to me not to have the "else if v6" as long as we are careful
with who can set this family :-)
We can drop the patch then.
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
982
days inactive
983
days old
10 comments
3 participants
participants (3)
-
Matthieu Baerts -
Paolo Abeni -
Peter Krystad