Re: [lwip-devel] [PATCH] PPP, PPPoE: Use service name and concentrator name

[Sylvain Rochet]

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