Re: Add missing #ifdefs to pppx_if_destroy()
On Thu, Mar 26, 2020 at 01:46:29PM +0100, Martin Pieuchot wrote:
> Does the diff below works for you? Are you ok with the direction? Any
> comment?
Diff works for me, Except you missed switch in the and of
pppx_add_session() and pipex_add_session().
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.76
diff -u -p -r1.76 if_pppx.c
--- sys/net/if_pppx.c 20 Feb 2020 16:56:52 - 1.76
+++ sys/net/if_pppx.c 26 Mar 2020 13:21:32 -
@@ -675,12 +675,9 @@ pppx_add_session(struct pppx_dev *pxd, s
return (EINVAL);
break;
#endif
-#ifdef PIPEX_PPTP
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
case PIPEX_PROTO_PPTP:
-#endif
-#ifdef PIPEX_L2TP
case PIPEX_PROTO_L2TP:
-#endif
switch (req->pr_peer_address.ss_family) {
case AF_INET:
if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
@@ -701,6 +698,7 @@ pppx_add_session(struct pppx_dev *pxd, s
req->pr_local_address.ss_len)
return (EINVAL);
break;
+#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
default:
return (EPROTONOSUPPORT);
}
@@ -854,6 +852,7 @@ pppx_add_session(struct pppx_dev *pxd, s
chain = PIPEX_ID_HASHTABLE(session->session_id);
LIST_INSERT_HEAD(chain, session, id_chain);
LIST_INSERT_HEAD(_session_list, session, session_list);
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
switch (req->pr_protocol) {
case PIPEX_PROTO_PPTP:
case PIPEX_PROTO_L2TP:
@@ -862,6 +861,7 @@ pppx_add_session(struct pppx_dev *pxd, s
LIST_INSERT_HEAD(chain, session, peer_addr_chain);
break;
}
+#endif
/* if first session is added, start timer */
if (LIST_NEXT(session, session_list) == NULL)
@@ -967,13 +967,14 @@ pppx_if_destroy(struct pppx_dev *pxd, st
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
switch (session->protocol) {
case PIPEX_PROTO_PPTP:
case PIPEX_PROTO_L2TP:
- LIST_REMOVE((struct pipex_session *)session,
- peer_addr_chain);
+ LIST_REMOVE(session, peer_addr_chain);
break;
}
+#endif
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(_session_list))
Index: sys/net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.108
diff -u -p -r1.108 pipex.c
--- sys/net/pipex.c 25 Mar 2020 11:39:58 - 1.108
+++ sys/net/pipex.c 26 Mar 2020 13:21:33 -
@@ -283,12 +283,8 @@ pipex_add_session(struct pipex_session_r
break;
#endif
#if defined(PIPEX_L2TP) || defined(PIPEX_PPTP)
-#ifdef PIPEX_PPTP
case PIPEX_PROTO_PPTP:
-#endif
-#ifdef PIPEX_L2TP
case PIPEX_PROTO_L2TP:
-#endif
switch (req->pr_peer_address.ss_family) {
case AF_INET:
if (req->pr_peer_address.ss_len !=
@@ -311,7 +307,7 @@ pipex_add_session(struct pipex_session_r
req->pr_local_address.ss_len)
return (EINVAL);
break;
-#endif
+#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
default:
return (EPROTONOSUPPORT);
}
@@ -450,6 +446,7 @@ pipex_add_session(struct pipex_session_r
chain = PIPEX_ID_HASHTABLE(session->session_id);
LIST_INSERT_HEAD(chain, session, id_chain);
LIST_INSERT_HEAD(_session_list, session, session_list);
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
switch (req->pr_protocol) {
case PIPEX_PROTO_PPTP:
case PIPEX_PROTO_L2TP:
@@ -457,6 +454,7 @@ pipex_add_session(struct pipex_session_r
pipex_sockaddr_hash_key(>peer.sa));
LIST_INSERT_HEAD(chain, session, peer_addr_chain);
}
+#endif
/* if first session is added, start timer */
if (LIST_NEXT(session, session_list) == NULL)
@@ -581,16 +579,15 @@ pipex_destroy_session(struct pipex_sessi
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
-#ifdef PIPEX_PPTP
- if (session->protocol == PIPEX_PROTO_PPTP) {
- LIST_REMOVE(session, peer_addr_chain);
- }
-#endif
-#ifdef PIPEX_L2TP
- if (session->protocol == PIPEX_PROTO_L2TP) {
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
+ switch (session->protocol) {
+ case PIPEX_PROTO_PPTP:
+ case PIPEX_PROTO_L2TP:
LIST_REMOVE(session, peer_addr_chain);
+ break;
}
#endif
+
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(_session_list))
Re: Add missing #ifdefs to pppx_if_destroy()
On 26/03/20(Thu) 14:41, Vitaliy Makkoveev wrote:
> On Thu, Mar 26, 2020 at 11:56:27AM +0100, Martin Pieuchot wrote:
> > On 26/03/20(Thu) 13:34, Vitaliy Makkoveev wrote:
> > > Add missing #ifdefs to pppx_if_destroy() as it done in
> > > pipex_destroy_session(). Also remove unnecessary cast.
> >
> > What's the point of such #ifdef? I understand the current code is not
> > coherent, but does this reduce the binary size? For a case in a switch?
> > Because it clearly complicates the understanding of the code.
> Code coherency is the goal.
> > So maybe having #if defined(PIPEX_PPTP) || defined(PIPEX_L2TP) there as
> > well as in pppx_add_session() and pipex_destroy_session() is the way to
> > go.
> >
> > But the underlying question would it make sense to have pppx_if_destroy()
> > and pipex_destroy_session() call the same function to clear sessions?
> >
> > The same could be add for pipex_add_session() and pppx_add_session().
> My next goal.
Great!
> I updated this diff with '#if defined()...' and for
> pipex_destroy_session() too.
Here's what I meant. To remove the individual #ifdef. This is just
compiled as I don't have a setup to test this change right now.
What's interesting in that change is that `peer_addr_chain' appears to
be only used by PPTP and L2TP. Is it so? If that the case what does
that implies about data structure organization?
Does the diff below works for you? Are you ok with the direction? Any
comment?
Index: net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.76
diff -u -p -r1.76 if_pppx.c
--- net/if_pppx.c 20 Feb 2020 16:56:52 - 1.76
+++ net/if_pppx.c 26 Mar 2020 12:38:53 -
@@ -675,12 +675,9 @@ pppx_add_session(struct pppx_dev *pxd, s
return (EINVAL);
break;
#endif
-#ifdef PIPEX_PPTP
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
case PIPEX_PROTO_PPTP:
-#endif
-#ifdef PIPEX_L2TP
case PIPEX_PROTO_L2TP:
-#endif
switch (req->pr_peer_address.ss_family) {
case AF_INET:
if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
@@ -701,6 +698,7 @@ pppx_add_session(struct pppx_dev *pxd, s
req->pr_local_address.ss_len)
return (EINVAL);
break;
+#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
default:
return (EPROTONOSUPPORT);
}
@@ -967,13 +965,14 @@ pppx_if_destroy(struct pppx_dev *pxd, st
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
switch (session->protocol) {
case PIPEX_PROTO_PPTP:
case PIPEX_PROTO_L2TP:
- LIST_REMOVE((struct pipex_session *)session,
- peer_addr_chain);
+ LIST_REMOVE(session, peer_addr_chain);
break;
}
+#endif
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(_session_list))
Index: net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.108
diff -u -p -r1.108 pipex.c
--- net/pipex.c 25 Mar 2020 11:39:58 - 1.108
+++ net/pipex.c 26 Mar 2020 12:41:09 -
@@ -283,12 +283,8 @@ pipex_add_session(struct pipex_session_r
break;
#endif
#if defined(PIPEX_L2TP) || defined(PIPEX_PPTP)
-#ifdef PIPEX_PPTP
case PIPEX_PROTO_PPTP:
-#endif
-#ifdef PIPEX_L2TP
case PIPEX_PROTO_L2TP:
-#endif
switch (req->pr_peer_address.ss_family) {
case AF_INET:
if (req->pr_peer_address.ss_len !=
@@ -311,7 +307,7 @@ pipex_add_session(struct pipex_session_r
req->pr_local_address.ss_len)
return (EINVAL);
break;
-#endif
+#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
default:
return (EPROTONOSUPPORT);
}
@@ -581,16 +577,15 @@ pipex_destroy_session(struct pipex_sessi
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
-#ifdef PIPEX_PPTP
- if (session->protocol == PIPEX_PROTO_PPTP) {
- LIST_REMOVE(session, peer_addr_chain);
- }
-#endif
-#ifdef PIPEX_L2TP
- if (session->protocol == PIPEX_PROTO_L2TP) {
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
+ switch (session->protocol) {
+ case PIPEX_PROTO_PPTP:
+ case PIPEX_PROTO_L2TP:
LIST_REMOVE(session, peer_addr_chain);
+ break;
}
#endif
+
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(_session_list))
pipex_timer_stop();
Re: Add missing #ifdefs to pppx_if_destroy()
On Thu, Mar 26, 2020 at 11:56:27AM +0100, Martin Pieuchot wrote:
> On 26/03/20(Thu) 13:34, Vitaliy Makkoveev wrote:
> > Add missing #ifdefs to pppx_if_destroy() as it done in
> > pipex_destroy_session(). Also remove unnecessary cast.
>
> What's the point of such #ifdef? I understand the current code is not
> coherent, but does this reduce the binary size? For a case in a switch?
> Because it clearly complicates the understanding of the code.
Code coherency is the goal.
> So maybe having #if defined(PIPEX_PPTP) || defined(PIPEX_L2TP) there as
> well as in pppx_add_session() and pipex_destroy_session() is the way to
> go.
>
> But the underlying question would it make sense to have pppx_if_destroy()
> and pipex_destroy_session() call the same function to clear sessions?
>
> The same could be add for pipex_add_session() and pppx_add_session().
My next goal.
I updated this diff with '#if defined()...' and for
pipex_destroy_session() too.
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.76
diff -u -p -r1.76 if_pppx.c
--- sys/net/if_pppx.c 20 Feb 2020 16:56:52 - 1.76
+++ sys/net/if_pppx.c 26 Mar 2020 11:27:07 -
@@ -967,13 +967,18 @@ pppx_if_destroy(struct pppx_dev *pxd, st
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
switch (session->protocol) {
+#ifdef PIPEX_PPTP
case PIPEX_PROTO_PPTP:
+#endif
+#ifdef PIPEX_L2TP
case PIPEX_PROTO_L2TP:
- LIST_REMOVE((struct pipex_session *)session,
- peer_addr_chain);
+#endif
+ LIST_REMOVE(session, peer_addr_chain);
break;
}
+#endif
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(_session_list))
Index: sys/net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.108
diff -u -p -r1.108 pipex.c
--- sys/net/pipex.c 25 Mar 2020 11:39:58 - 1.108
+++ sys/net/pipex.c 26 Mar 2020 11:27:08 -
@@ -581,16 +581,19 @@ pipex_destroy_session(struct pipex_sessi
LIST_REMOVE(session, id_chain);
LIST_REMOVE(session, session_list);
+#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
+ switch (session->protocol) {
#ifdef PIPEX_PPTP
- if (session->protocol == PIPEX_PROTO_PPTP) {
- LIST_REMOVE(session, peer_addr_chain);
- }
+ case PIPEX_PROTO_PPTP:
#endif
#ifdef PIPEX_L2TP
- if (session->protocol == PIPEX_PROTO_L2TP) {
+ case PIPEX_PROTO_L2TP:
+#endif
LIST_REMOVE(session, peer_addr_chain);
+ break;
}
#endif
+
/* if final session is destroyed, stop timer */
if (LIST_EMPTY(_session_list))
pipex_timer_stop();
Re: Add missing #ifdefs to pppx_if_destroy()
On 26/03/20(Thu) 13:34, Vitaliy Makkoveev wrote:
> Add missing #ifdefs to pppx_if_destroy() as it done in
> pipex_destroy_session(). Also remove unnecessary cast.
What's the point of such #ifdef? I understand the current code is not
coherent, but does this reduce the binary size? For a case in a switch?
Because it clearly complicates the understanding of the code.
If one is going to remove support for any of the two, grepping for
PIPEX_PROTO_* will be necessary.
So maybe having #if defined(PIPEX_PPTP) || defined(PIPEX_L2TP) there as
well as in pppx_add_session() and pipex_destroy_session() is the way to
go.
But the underlying question would it make sense to have pppx_if_destroy()
and pipex_destroy_session() call the same function to clear sessions?
The same could be add for pipex_add_session() and pppx_add_session().
Any cleanup here is welcome, building understanding of the data
structures used by those pseudo-drivers is the way to get they out of
the KERNEL_LOCK(). If you're curious the entry point is pipexintr().
Getting that out of KERNEL_LOCK() will at least improve latency of the
system and is a required step to improve performances.
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.76
> diff -u -p -r1.76 if_pppx.c
> --- sys/net/if_pppx.c 20 Feb 2020 16:56:52 - 1.76
> +++ sys/net/if_pppx.c 26 Mar 2020 10:07:26 -
> @@ -967,13 +967,14 @@ pppx_if_destroy(struct pppx_dev *pxd, st
>
> LIST_REMOVE(session, id_chain);
> LIST_REMOVE(session, session_list);
> - switch (session->protocol) {
> - case PIPEX_PROTO_PPTP:
> - case PIPEX_PROTO_L2TP:
> - LIST_REMOVE((struct pipex_session *)session,
> - peer_addr_chain);
> - break;
> - }
> +#ifdef PIPEX_PPTP
> + if (session->protocol == PIPEX_PROTO_PPTP)
> + LIST_REMOVE(session, peer_addr_chain);
> +#endif
> +#ifdef PIPEX_L2TP
> + if (session->protocol == PIPEX_PROTO_L2TP)
> + LIST_REMOVE(session, peer_addr_chain);
> +#endif
>
> /* if final session is destroyed, stop timer */
> if (LIST_EMPTY(_session_list))
>
