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 -0000 1.76
+++ net/if_pppx.c 26 Mar 2020 12:38:53 -0000
@@ -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(&pipex_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 -0000 1.108
+++ net/pipex.c 26 Mar 2020 12:41:09 -0000
@@ -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(&pipex_session_list))
pipex_timer_stop();
