Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

[Guillaume Nault]

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  wrote:
> > On Thu, Aug 18, 2016 at 09:59:03AM +0800, f...@ikuai8.com wrote:
> >> From: Gao Feng 
> >>
> >> 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 
> >> ---
> >>  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.

Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

[Feng Gao]

inline.

On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault  wrote:
> On Thu, Aug 18, 2016 at 09:59:03AM +0800, f...@ikuai8.com wrote:
>> From: Gao Feng 
>>
>> 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 
>> ---
>>  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

Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

[Guillaume Nault]

On Thu, Aug 18, 2016 at 09:59:03AM +0800, f...@ikuai8.com wrote:
> From: Gao Feng 
> 
> 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 
> ---
>  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.

Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

[Philp Prindeville]

Reviewed-by: Philip Prindeville 


On 08/17/2016 07:59 PM, f...@48lvckh6395k16k5.yundunddos.com wrote:

From: Gao Feng

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

[PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

[fgao]

From: Gao Feng 

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 
---
 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