* [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation
@ 2016-08-18 1:59 fgao
2016-08-18 14:17 ` Philp Prindeville
2016-08-18 17:44 ` Guillaume Nault
0 siblings, 2 replies; 5+ messages in thread
From: fgao @ 2016-08-18 1:59 UTC (permalink / raw)
To: mostrows, jchapman, davem, philipp, netdev; +Cc: gfree.wind, Gao Feng
From: Gao Feng <fgao@ikuai8.com>
There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
as the value including assignment and condition check.
They should keep consistent with other codes.
Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
v1: Initial Patch
drivers/net/ppp/pppoe.c | 2 +-
net/l2tp/l2tp_ppp.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 4ddae81..684b773 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
goto err_put;
}
- sk->sk_state = PPPOX_CONNECTED;
+ sk->sk_state |= PPPOX_CONNECTED;
}
po->num = sp->sa_addr.pppoe.sid;
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index d9560aa..3984385 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -774,7 +774,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
out_no_ppp:
/* This is how we get the session context from the socket. */
sk->sk_user_data = session;
- sk->sk_state = PPPOX_CONNECTED;
+ sk->sk_state |= PPPOX_CONNECTED;
l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: created\n",
session->name);
@@ -856,7 +856,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
error = -ENOTCONN;
if (sk == NULL)
goto end;
- if (sk->sk_state != PPPOX_CONNECTED)
+ if (!(sk->sk_state & PPPOX_CONNECTED))
goto end;
error = -EBADF;
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation
2016-08-18 1:59 [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation fgao
@ 2016-08-18 14:17 ` Philp Prindeville
2016-08-18 17:44 ` Guillaume Nault
1 sibling, 0 replies; 5+ messages in thread
From: Philp Prindeville @ 2016-08-18 14:17 UTC (permalink / raw)
To: fgao, mostrows, jchapman, davem, netdev; +Cc: gfree.wind
Reviewed-by: Philip Prindeville <philipp@redfish-solutions.com>
On 08/17/2016 07:59 PM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
> From: Gao Feng<fgao@ikuai8.com>
>
> There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
> as the value including assignment and condition check.
> They should keep consistent with other codes.
>
> Signed-off-by: Gao Feng<fgao@ikuai8.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation
2016-08-18 1:59 [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation fgao
2016-08-18 14:17 ` Philp Prindeville
@ 2016-08-18 17:44 ` Guillaume Nault
2016-08-18 22:58 ` Feng Gao
1 sibling, 1 reply; 5+ messages in thread
From: Guillaume Nault @ 2016-08-18 17:44 UTC (permalink / raw)
To: fgao; +Cc: mostrows, jchapman, davem, philipp, netdev, gfree.wind
On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
> as the value including assignment and condition check.
> They should keep consistent with other codes.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
> v1: Initial Patch
>
> drivers/net/ppp/pppoe.c | 2 +-
> net/l2tp/l2tp_ppp.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 4ddae81..684b773 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
> goto err_put;
> }
>
> - sk->sk_state = PPPOX_CONNECTED;
> + sk->sk_state |= PPPOX_CONNECTED;
>
Using plain assignment makes it clear for the reader that other flags
are unset. I see no reason for changing this.
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index d9560aa..3984385 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -774,7 +774,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
> out_no_ppp:
> /* This is how we get the session context from the socket. */
> sk->sk_user_data = session;
> - sk->sk_state = PPPOX_CONNECTED;
> + sk->sk_state |= PPPOX_CONNECTED;
>
Same here.
> @@ -856,7 +856,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
> error = -ENOTCONN;
> if (sk == NULL)
> goto end;
> - if (sk->sk_state != PPPOX_CONNECTED)
> + if (!(sk->sk_state & PPPOX_CONNECTED))
>
Looks like it was a bug. This one is worth a separate patch.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation
2016-08-18 17:44 ` Guillaume Nault
@ 2016-08-18 22:58 ` Feng Gao
2016-08-19 20:18 ` Guillaume Nault
0 siblings, 1 reply; 5+ messages in thread
From: Feng Gao @ 2016-08-18 22:58 UTC (permalink / raw)
To: Guillaume Nault
Cc: Gao Feng, mostrows, jchapman, David S. Miller, Philp Prindeville,
Linux Kernel Network Developers
inline.
On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault <g.nault@alphalink.fr> wrote:
> On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
>> as the value including assignment and condition check.
>> They should keep consistent with other codes.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>> v1: Initial Patch
>>
>> drivers/net/ppp/pppoe.c | 2 +-
>> net/l2tp/l2tp_ppp.c | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
>> index 4ddae81..684b773 100644
>> --- a/drivers/net/ppp/pppoe.c
>> +++ b/drivers/net/ppp/pppoe.c
>> @@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
>> goto err_put;
>> }
>>
>> - sk->sk_state = PPPOX_CONNECTED;
>> + sk->sk_state |= PPPOX_CONNECTED;
>>
> Using plain assignment makes it clear for the reader that other flags
> are unset. I see no reason for changing this.
I get you. So I don't modify the PPPOX_DEAD assignment.
But I am afraid if there is some case that the flag PPPOX_BOUND is set
before PPPOX_CONNECTED . Then the assignment of "PPPOX_CONNECTED" will
clear the PPPOX_BOUND flag.
>
>> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
>> index d9560aa..3984385 100644
>> --- a/net/l2tp/l2tp_ppp.c
>> +++ b/net/l2tp/l2tp_ppp.c
>> @@ -774,7 +774,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
>> out_no_ppp:
>> /* This is how we get the session context from the socket. */
>> sk->sk_user_data = session;
>> - sk->sk_state = PPPOX_CONNECTED;
>> + sk->sk_state |= PPPOX_CONNECTED;
>>
> Same here.
>
>> @@ -856,7 +856,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
>> error = -ENOTCONN;
>> if (sk == NULL)
>> goto end;
>> - if (sk->sk_state != PPPOX_CONNECTED)
>> + if (!(sk->sk_state & PPPOX_CONNECTED))
>>
> Looks like it was a bug. This one is worth a separate patch.
Ok, I send another patch for this bug.
Regards
Feng
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation
2016-08-18 22:58 ` Feng Gao
@ 2016-08-19 20:18 ` Guillaume Nault
0 siblings, 0 replies; 5+ messages in thread
From: Guillaume Nault @ 2016-08-19 20:18 UTC (permalink / raw)
To: Feng Gao
Cc: Gao Feng, mostrows, jchapman, David S. Miller, Philp Prindeville,
Linux Kernel Network Developers
On Fri, Aug 19, 2016 at 06:58:58AM +0800, Feng Gao wrote:
> inline.
>
> On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault <g.nault@alphalink.fr> wrote:
> > On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:
> >> From: Gao Feng <fgao@ikuai8.com>
> >>
> >> There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
> >> as the value including assignment and condition check.
> >> They should keep consistent with other codes.
> >>
> >> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> >> ---
> >> v1: Initial Patch
> >>
> >> drivers/net/ppp/pppoe.c | 2 +-
> >> net/l2tp/l2tp_ppp.c | 4 ++--
> >> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> >> index 4ddae81..684b773 100644
> >> --- a/drivers/net/ppp/pppoe.c
> >> +++ b/drivers/net/ppp/pppoe.c
> >> @@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
> >> goto err_put;
> >> }
> >>
> >> - sk->sk_state = PPPOX_CONNECTED;
> >> + sk->sk_state |= PPPOX_CONNECTED;
> >>
> > Using plain assignment makes it clear for the reader that other flags
> > are unset. I see no reason for changing this.
>
> I get you. So I don't modify the PPPOX_DEAD assignment.
> But I am afraid if there is some case that the flag PPPOX_BOUND is set
> before PPPOX_CONNECTED . Then the assignment of "PPPOX_CONNECTED" will
> clear the PPPOX_BOUND flag.
>
PPPOX_BOUND shouldn't be set here. If the socket hasn't been connected
before, it can't have the BOUND flag (pppox_ioctl(PPPIOCGCHAN) would
fail). But if it was connected, then it'd have to go through the
'/* Delete the old binding */' part of pppoe_connect() first, thus
reseting sk_state to PPPOX_NONE.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-19 20:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-18 1:59 [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation fgao
2016-08-18 14:17 ` Philp Prindeville
2016-08-18 17:44 ` Guillaume Nault
2016-08-18 22:58 ` Feng Gao
2016-08-19 20:18 ` Guillaume Nault
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.