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 -0000 1.76
> +++ sys/net/if_pppx.c 26 Mar 2020 10:07:26 -0000
> @@ -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(&pipex_session_list))
>
