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

inline.

On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault [off-list ref] wrote:

On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@ikuai8.com wrote:

quoted

From: Gao Feng <redacted>

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

quoted

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.

quoted

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