Re: Dedulpicate pipex(4) and pppx(4) code
On Thu, Apr 02, 2020 at 09:26:23AM +0200, Martin Pieuchot wrote:
> Hello Vitaliy,
>
> On 01/04/20(Wed) 12:59, Vitaliy Makkoveev wrote:
> > Updated diff. The idea is to use already existing pipex API for
> > pipex_session creation and destruction. pppx_if now holds a reference
> > to pipex_session.
>
> This is great!
>
> There are many things in this diff which makes it complicated to review,
> at least to me. Note that I'm not really familiar with this code.
>
> For example the panic() after the RBT_REMOVE() is now questionable since
> the lock is released then retaken. Which brings a question for later:
> what is the lock really protecting? That can be documented in the header
> like it is done for other data structures.
>
> So the changes includes:
>
> - pool_get() with PR_WAITOK should never fail. This could be the first
> step and also move the allocation at the beginning to make a pattern
> appear.
>
> - Allocation of `pxi_session' separately from `pxi', this creates quite
> some noise because of the line changing indirection.
>
> - Change in the error paths, which aren't correct and IMHO should be
> left out for the moment.
>
> - Use of pipex_add_session(), isn't that independent from the allocation
> of `session'?
>
> Could you help me review this by submitting smaller diffs? Thanks a
> lot!
Let's take a break, another pipex(4) issue found. I'll send diff for it
it today later with new thread.
>
> >
> > Index: sys/net/if_pppx.c
> > ===
> > RCS file: /cvs/src/sys/net/if_pppx.c,v
> > retrieving revision 1.78
> > diff -u -p -r1.78 if_pppx.c
> > --- sys/net/if_pppx.c 1 Apr 2020 07:15:59 - 1.78
> > +++ sys/net/if_pppx.c 1 Apr 2020 09:50:19 -
> > @@ -155,7 +155,7 @@ struct pppx_if {
> > int pxi_unit;
> > struct ifnetpxi_if;
> > struct pppx_dev *pxi_dev;
> > - struct pipex_sessionpxi_session;
> > + struct pipex_session*pxi_session;
> > struct pipex_iface_context pxi_ifcontext;
> > };
> >
> > @@ -655,15 +655,10 @@ int
> > pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
> > {
> > struct pppx_if *pxi;
> > - struct pipex_session *session;
> > - struct pipex_hash_head *chain;
> > struct ifnet *ifp;
> > int unit, error = 0;
> > struct in_ifaddr *ia;
> > struct sockaddr_in ifaddr;
> > -#ifdef PIPEX_PPPOE
> > - struct ifnet *over_ifp = NULL;
> > -#endif
> >
> > /*
> > * XXX: As long as `session' is allocated as part of a `pxi'
> > @@ -673,157 +668,16 @@ pppx_add_session(struct pppx_dev *pxd, s
> > if (req->pr_timeout_sec != 0)
> > return (EINVAL);
> >
> > - switch (req->pr_protocol) {
> > -#ifdef PIPEX_PPPOE
> > - case PIPEX_PROTO_PPPOE:
> > - over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
> > - if (over_ifp == NULL)
> > - return (EINVAL);
> > - if (req->pr_peer_address.ss_family != AF_UNSPEC)
> > - return (EINVAL);
> > - break;
> > -#endif
> > -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> > - case PIPEX_PROTO_PPTP:
> > - case PIPEX_PROTO_L2TP:
> > - switch (req->pr_peer_address.ss_family) {
> > - case AF_INET:
> > - if (req->pr_peer_address.ss_len != sizeof(struct
> > sockaddr_in))
> > - return (EINVAL);
> > - break;
> > -#ifdef INET6
> > - case AF_INET6:
> > - if (req->pr_peer_address.ss_len != sizeof(struct
> > sockaddr_in6))
> > - return (EINVAL);
> > - break;
> > -#endif
> > - default:
> > - return (EPROTONOSUPPORT);
> > - }
> > - if (req->pr_peer_address.ss_family !=
> > - req->pr_local_address.ss_family ||
> > - req->pr_peer_address.ss_len !=
> > - req->pr_local_address.ss_len)
> > - return (EINVAL);
> > - break;
> > -#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
> > - default:
> > - return (EPROTONOSUPPORT);
> > - }
> > -
> > pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
> > - if (pxi == NULL)
> > - return (ENOMEM);
> > -
> > - session = >pxi_session;
> > ifp = >pxi_if;
> >
> > - /* fake a pipex interface context */
> > - session->pipex_iface = >pxi_ifcontext;
> > - session->pipex_iface->ifnet_this = ifp;
> > - session->pipex_iface->pipexmode = PIPEX_ENABLED;
> > -
> > - /* setup session */
> > - session->state = PIPEX_STATE_OPENED;
> > - session->protocol = req->pr_protocol;
> > - session->session_id = req->pr_session_id;
> > - session->peer_session_id = req->pr_peer_session_id;
> > - session->peer_mru = req->pr_peer_mru;
> > - session->timeout_sec = req->pr_timeout_sec;
> > - session->ppp_flags =
Re: Dedulpicate pipex(4) and pppx(4) code
Hello Vitaliy,
On 01/04/20(Wed) 12:59, Vitaliy Makkoveev wrote:
> Updated diff. The idea is to use already existing pipex API for
> pipex_session creation and destruction. pppx_if now holds a reference
> to pipex_session.
This is great!
There are many things in this diff which makes it complicated to review,
at least to me. Note that I'm not really familiar with this code.
For example the panic() after the RBT_REMOVE() is now questionable since
the lock is released then retaken. Which brings a question for later:
what is the lock really protecting? That can be documented in the header
like it is done for other data structures.
So the changes includes:
- pool_get() with PR_WAITOK should never fail. This could be the first
step and also move the allocation at the beginning to make a pattern
appear.
- Allocation of `pxi_session' separately from `pxi', this creates quite
some noise because of the line changing indirection.
- Change in the error paths, which aren't correct and IMHO should be
left out for the moment.
- Use of pipex_add_session(), isn't that independent from the allocation
of `session'?
Could you help me review this by submitting smaller diffs? Thanks a
lot!
>
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 if_pppx.c
> --- sys/net/if_pppx.c 1 Apr 2020 07:15:59 - 1.78
> +++ sys/net/if_pppx.c 1 Apr 2020 09:50:19 -
> @@ -155,7 +155,7 @@ struct pppx_if {
> int pxi_unit;
> struct ifnetpxi_if;
> struct pppx_dev *pxi_dev;
> - struct pipex_sessionpxi_session;
> + struct pipex_session*pxi_session;
> struct pipex_iface_context pxi_ifcontext;
> };
>
> @@ -655,15 +655,10 @@ int
> pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
> {
> struct pppx_if *pxi;
> - struct pipex_session *session;
> - struct pipex_hash_head *chain;
> struct ifnet *ifp;
> int unit, error = 0;
> struct in_ifaddr *ia;
> struct sockaddr_in ifaddr;
> -#ifdef PIPEX_PPPOE
> - struct ifnet *over_ifp = NULL;
> -#endif
>
> /*
>* XXX: As long as `session' is allocated as part of a `pxi'
> @@ -673,157 +668,16 @@ pppx_add_session(struct pppx_dev *pxd, s
> if (req->pr_timeout_sec != 0)
> return (EINVAL);
>
> - switch (req->pr_protocol) {
> -#ifdef PIPEX_PPPOE
> - case PIPEX_PROTO_PPPOE:
> - over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
> - if (over_ifp == NULL)
> - return (EINVAL);
> - if (req->pr_peer_address.ss_family != AF_UNSPEC)
> - return (EINVAL);
> - break;
> -#endif
> -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> - case PIPEX_PROTO_PPTP:
> - case PIPEX_PROTO_L2TP:
> - switch (req->pr_peer_address.ss_family) {
> - case AF_INET:
> - if (req->pr_peer_address.ss_len != sizeof(struct
> sockaddr_in))
> - return (EINVAL);
> - break;
> -#ifdef INET6
> - case AF_INET6:
> - if (req->pr_peer_address.ss_len != sizeof(struct
> sockaddr_in6))
> - return (EINVAL);
> - break;
> -#endif
> - default:
> - return (EPROTONOSUPPORT);
> - }
> - if (req->pr_peer_address.ss_family !=
> - req->pr_local_address.ss_family ||
> - req->pr_peer_address.ss_len !=
> - req->pr_local_address.ss_len)
> - return (EINVAL);
> - break;
> -#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
> - default:
> - return (EPROTONOSUPPORT);
> - }
> -
> pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
> - if (pxi == NULL)
> - return (ENOMEM);
> -
> - session = >pxi_session;
> ifp = >pxi_if;
>
> - /* fake a pipex interface context */
> - session->pipex_iface = >pxi_ifcontext;
> - session->pipex_iface->ifnet_this = ifp;
> - session->pipex_iface->pipexmode = PIPEX_ENABLED;
> -
> - /* setup session */
> - session->state = PIPEX_STATE_OPENED;
> - session->protocol = req->pr_protocol;
> - session->session_id = req->pr_session_id;
> - session->peer_session_id = req->pr_peer_session_id;
> - session->peer_mru = req->pr_peer_mru;
> - session->timeout_sec = req->pr_timeout_sec;
> - session->ppp_flags = req->pr_ppp_flags;
> - session->ppp_id = req->pr_ppp_id;
> -
> - session->ip_forward = 1;
> -
> - session->ip_address.sin_family = AF_INET;
> - session->ip_address.sin_len = sizeof(struct sockaddr_in);
> - session->ip_address.sin_addr = req->pr_ip_address;
> -
> - session->ip_netmask.sin_family
Re: Dedulpicate pipex(4) and pppx(4) code
Updated diff. The idea is to use already existing pipex API for
pipex_session creation and destruction. pppx_if now holds a reference
to pipex_session.
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.78
diff -u -p -r1.78 if_pppx.c
--- sys/net/if_pppx.c 1 Apr 2020 07:15:59 - 1.78
+++ sys/net/if_pppx.c 1 Apr 2020 09:50:19 -
@@ -155,7 +155,7 @@ struct pppx_if {
int pxi_unit;
struct ifnetpxi_if;
struct pppx_dev *pxi_dev;
- struct pipex_sessionpxi_session;
+ struct pipex_session*pxi_session;
struct pipex_iface_context pxi_ifcontext;
};
@@ -655,15 +655,10 @@ int
pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
{
struct pppx_if *pxi;
- struct pipex_session *session;
- struct pipex_hash_head *chain;
struct ifnet *ifp;
int unit, error = 0;
struct in_ifaddr *ia;
struct sockaddr_in ifaddr;
-#ifdef PIPEX_PPPOE
- struct ifnet *over_ifp = NULL;
-#endif
/*
* XXX: As long as `session' is allocated as part of a `pxi'
@@ -673,157 +668,16 @@ pppx_add_session(struct pppx_dev *pxd, s
if (req->pr_timeout_sec != 0)
return (EINVAL);
- switch (req->pr_protocol) {
-#ifdef PIPEX_PPPOE
- case PIPEX_PROTO_PPPOE:
- over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
- if (over_ifp == NULL)
- return (EINVAL);
- if (req->pr_peer_address.ss_family != AF_UNSPEC)
- return (EINVAL);
- break;
-#endif
-#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
- case PIPEX_PROTO_PPTP:
- case PIPEX_PROTO_L2TP:
- switch (req->pr_peer_address.ss_family) {
- case AF_INET:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
- return (EINVAL);
- break;
-#ifdef INET6
- case AF_INET6:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in6))
- return (EINVAL);
- break;
-#endif
- default:
- return (EPROTONOSUPPORT);
- }
- if (req->pr_peer_address.ss_family !=
- req->pr_local_address.ss_family ||
- req->pr_peer_address.ss_len !=
- req->pr_local_address.ss_len)
- return (EINVAL);
- break;
-#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
- default:
- return (EPROTONOSUPPORT);
- }
-
pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
- if (pxi == NULL)
- return (ENOMEM);
-
- session = >pxi_session;
ifp = >pxi_if;
- /* fake a pipex interface context */
- session->pipex_iface = >pxi_ifcontext;
- session->pipex_iface->ifnet_this = ifp;
- session->pipex_iface->pipexmode = PIPEX_ENABLED;
-
- /* setup session */
- session->state = PIPEX_STATE_OPENED;
- session->protocol = req->pr_protocol;
- session->session_id = req->pr_session_id;
- session->peer_session_id = req->pr_peer_session_id;
- session->peer_mru = req->pr_peer_mru;
- session->timeout_sec = req->pr_timeout_sec;
- session->ppp_flags = req->pr_ppp_flags;
- session->ppp_id = req->pr_ppp_id;
-
- session->ip_forward = 1;
-
- session->ip_address.sin_family = AF_INET;
- session->ip_address.sin_len = sizeof(struct sockaddr_in);
- session->ip_address.sin_addr = req->pr_ip_address;
-
- session->ip_netmask.sin_family = AF_INET;
- session->ip_netmask.sin_len = sizeof(struct sockaddr_in);
- session->ip_netmask.sin_addr = req->pr_ip_netmask;
-
- if (session->ip_netmask.sin_addr.s_addr == 0L)
- session->ip_netmask.sin_addr.s_addr = 0xL;
- session->ip_address.sin_addr.s_addr &=
- session->ip_netmask.sin_addr.s_addr;
-
- if (req->pr_peer_address.ss_len > 0)
- memcpy(>peer, >pr_peer_address,
- MIN(req->pr_peer_address.ss_len, sizeof(session->peer)));
- if (req->pr_local_address.ss_len > 0)
- memcpy(>local, >pr_local_address,
- MIN(req->pr_local_address.ss_len, sizeof(session->local)));
-#ifdef PIPEX_PPPOE
- if (req->pr_protocol == PIPEX_PROTO_PPPOE)
- session->proto.pppoe.over_ifidx = over_ifp->if_index;
-#endif
-#ifdef PIPEX_PPTP
- if (req->pr_protocol == PIPEX_PROTO_PPTP) {
- struct pipex_pptp_session *sess_pptp = >proto.pptp;
-
- sess_pptp->snd_gap = 0;
- sess_pptp->rcv_gap = 0;
- sess_pptp->snd_una =
Re: Dedulpicate pipex(4) and pppx(4) code
ping
Dedulpicate pipex(4) and pppx(4) code
pppx(4) has code copypasted from pipex(4). Patch below deduplicates it.
Introduded pipex_session_setup() and pipex_session_destroy() functions.
Original pipex_destroy_session() renamed to pipex_del_session() to be
consistent with PIPEXDSESSION (Delete the specified session from the
kernel).
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.77
diff -u -p -r1.77 if_pppx.c
--- sys/net/if_pppx.c 26 Mar 2020 16:50:46 - 1.77
+++ sys/net/if_pppx.c 28 Mar 2020 14:45:08 -
@@ -655,167 +655,26 @@ int
pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
{
struct pppx_if *pxi;
- struct pipex_session *session;
- struct pipex_hash_head *chain;
+ struct pipex_iface_context *iface;
struct ifnet *ifp;
- int unit, error = 0;
+ int unit, error;
struct in_ifaddr *ia;
struct sockaddr_in ifaddr;
-#ifdef PIPEX_PPPOE
- struct ifnet *over_ifp = NULL;
-#endif
- switch (req->pr_protocol) {
-#ifdef PIPEX_PPPOE
- case PIPEX_PROTO_PPPOE:
- over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
- if (over_ifp == NULL)
- return (EINVAL);
- if (req->pr_peer_address.ss_family != AF_UNSPEC)
- return (EINVAL);
- break;
-#endif
-#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
- case PIPEX_PROTO_PPTP:
- case PIPEX_PROTO_L2TP:
- switch (req->pr_peer_address.ss_family) {
- case AF_INET:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
- return (EINVAL);
- break;
-#ifdef INET6
- case AF_INET6:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in6))
- return (EINVAL);
- break;
-#endif
- default:
- return (EPROTONOSUPPORT);
- }
- if (req->pr_peer_address.ss_family !=
- req->pr_local_address.ss_family ||
- req->pr_peer_address.ss_len !=
- req->pr_local_address.ss_len)
- return (EINVAL);
- break;
-#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
- default:
- return (EPROTONOSUPPORT);
- }
+ NET_ASSERT_LOCKED();
pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
- if (pxi == NULL)
- return (ENOMEM);
- session = >pxi_session;
ifp = >pxi_if;
+ iface = >pxi_ifcontext;
- /* fake a pipex interface context */
- session->pipex_iface = >pxi_ifcontext;
- session->pipex_iface->ifnet_this = ifp;
- session->pipex_iface->pipexmode = PIPEX_ENABLED;
-
- /* setup session */
- session->state = PIPEX_STATE_OPENED;
- session->protocol = req->pr_protocol;
- session->session_id = req->pr_session_id;
- session->peer_session_id = req->pr_peer_session_id;
- session->peer_mru = req->pr_peer_mru;
- session->timeout_sec = req->pr_timeout_sec;
- session->ppp_flags = req->pr_ppp_flags;
- session->ppp_id = req->pr_ppp_id;
-
- session->ip_forward = 1;
-
- session->ip_address.sin_family = AF_INET;
- session->ip_address.sin_len = sizeof(struct sockaddr_in);
- session->ip_address.sin_addr = req->pr_ip_address;
-
- session->ip_netmask.sin_family = AF_INET;
- session->ip_netmask.sin_len = sizeof(struct sockaddr_in);
- session->ip_netmask.sin_addr = req->pr_ip_netmask;
-
- if (session->ip_netmask.sin_addr.s_addr == 0L)
- session->ip_netmask.sin_addr.s_addr = 0xL;
- session->ip_address.sin_addr.s_addr &=
- session->ip_netmask.sin_addr.s_addr;
-
- if (req->pr_peer_address.ss_len > 0)
- memcpy(>peer, >pr_peer_address,
- MIN(req->pr_peer_address.ss_len, sizeof(session->peer)));
- if (req->pr_local_address.ss_len > 0)
- memcpy(>local, >pr_local_address,
- MIN(req->pr_local_address.ss_len, sizeof(session->local)));
-#ifdef PIPEX_PPPOE
- if (req->pr_protocol == PIPEX_PROTO_PPPOE)
- session->proto.pppoe.over_ifidx = over_ifp->if_index;
-#endif
-#ifdef PIPEX_PPTP
- if (req->pr_protocol == PIPEX_PROTO_PPTP) {
- struct pipex_pptp_session *sess_pptp = >proto.pptp;
-
- sess_pptp->snd_gap = 0;
- sess_pptp->rcv_gap = 0;
- sess_pptp->snd_una = req->pr_proto.pptp.snd_una;
- sess_pptp->snd_nxt = req->pr_proto.pptp.snd_nxt;
- sess_pptp->rcv_nxt = req->pr_proto.pptp.rcv_nxt;
- sess_pptp->rcv_acked = req->pr_proto.pptp.rcv_acked;
-
- sess_pptp->winsz =
