[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [lwip-devel] [PATCH] PPP, PPPoE: Use service name and concentrator n
From: |
Sylvain Rochet |
Subject: |
Re: [lwip-devel] [PATCH] PPP, PPPoE: Use service name and concentrator name |
Date: |
Thu, 20 Dec 2018 01:09:45 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
Hi Jacob,
On Wed, Dec 19, 2018 at 10:24:57PM +0100, Jacob Kroon wrote:
> Make pppoe_create() actually store the passed service name and
> concentrator name, so that they are passed in the PADI/PADR/PADS packets.
> ---
> src/netif/ppp/pppoe.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/netif/ppp/pppoe.c b/src/netif/ppp/pppoe.c
> index 47ec95b1..8cfc54c4 100644
> --- a/src/netif/ppp/pppoe.c
> +++ b/src/netif/ppp/pppoe.c
> @@ -175,8 +175,8 @@ ppp_pcb *pppoe_create(struct netif *pppif,
> {
> ppp_pcb *ppp;
> struct pppoe_softc *sc;
> - LWIP_UNUSED_ARG(service_name);
> - LWIP_UNUSED_ARG(concentrator_name);
> + LWIP_UNUSED_ARG(service_name); /* if !PPPOE_SCNAME_SUPPORT */
> + LWIP_UNUSED_ARG(concentrator_name); /* if !PPPOE_SCNAME_SUPPORT */
I prefer enclosing them with so we still get warnings if the feature is
enabled:
#if !PPPOE_SCNAME_SUPPORT
LWIP_UNUSED_ARG(service_name);
LWIP_UNUSED_ARG(concentrator_name);
#endif /* !PPPOE_SCNAME_SUPPORT */
> LWIP_ASSERT_CORE_LOCKED();
>
> sc = (struct pppoe_softc *)LWIP_MEMPOOL_ALLOC(PPPOE_IF);
> @@ -193,6 +193,12 @@ ppp_pcb *pppoe_create(struct netif *pppif,
> memset(sc, 0, sizeof(struct pppoe_softc));
> sc->pcb = ppp;
> sc->sc_ethif = ethif;
> +#if PPPOE_SCNAME_SUPPORT
> + if (service_name != NULL)
> + sc->sc_service_name = strdup(service_name);
> + if (concentrator_name != NULL)
> + sc->sc_concentrator_name = strdup(concentrator_name);
> +#endif /* PPPOE_SCNAME_SUPPORT */
We can't use strdup here, sc->sc_service_name and
sc->sc_concentrator_name are currently free'd using mem_free() which
usually don't share its heap with the libc one.
Anyway, I would prefer avoiding the copy at all, just copying the
strings pointers and assuming user application is not going to free
those strings is acceptable, as they probably come from a configuration
struct that is not going to disappear, furthermore this is what we
already do in ppp_set_auth() or pppol2tp_create().
Nitpick, because I would fix that anyway before pushing the patch, but
lwIP coding style makes braces {} mandatory even for single line if
statement.
Thank you for working on that ! :)
Sylvain
signature.asc
Description: Digital signature