Re: [lwip-devel] IPv6 L2TP PPP review

[Sylvain Rochet]

Hello Ivan,


On Mon, Mar 02, 2015 at 09:28:16AM -0700, Ivan Delamer wrote:
> Hey Sylvain,
> 
> After a quick look it seems OK to me, but I don't understand l2tp
> well enough to be 100% certain.

L2TP is not special at all related to UDP, that's just plain UDP, the 
only "tricky" part is that the peer might answer with its own random 
source port.

Is the following correct ? :

Create v4:

  struct udp_pcb *udp = udp_new();
  udp_recv(udp, pppol2tp_input_ip4, l2tp);

Create v6:

  struct udp_pcb *udp = udp_new_ip6();
  udp_recv_ip6(udp, pppol2tp_input_ip6, l2tp);


I am using two differents callbacks (_ip4 vs _ip6). I am pretty sure 
this is not mandatory by the way UDP callbacks are currently designed 
but it looks like to be the spirit of the IPv6 API.

Is using a common callbacks function for both v4 and v6 using 
PCB_ISIPV6() and casts for ip_addr_t* and ip6_addr_t* in the callback 
considered API safe ? (i.e. not a clever way of using the API from what 
it is currently doing but which might change in the future).


Is dual-stack bind correct ? :

  struct udp_pcb *udp;

  if (PCB_ISIPV6(udp)) {
    udp_bind_ip6(udp, IP6_ADDR_ANY, 0);
  } else {
    udp_bind(udp, IP_ADDR_ANY, 0);
  }


Is dual-stack udp_sendto correct ? :

  ipX_addr_t remote_ip;
  u16_t port;
  struct udp_pcb *udp;
  struct netif *netif;

  if (PCB_ISIPV6(udp)) {
    if (netif) {
      udp_sendto_if_ip6(udp, pb, &remote_ip.ip6, port, netif);
    } else {
      udp_sendto_ip6(udp, pb, &remote_ip.ip6, port);
    }
  } else {
    if (netif) {
      err = udp_sendto_if(udp, pb, &remote_ip.ip4, port, netif);
    } else {
     err = udp_sendto(udp, pb, &remote_ip.ip4, port);
    }
  }


Is udp remove correct for both v4 and v6 ? :

udp_remove(udp);


> If you're not sure, just wrap the code with a #define 0 or something
> so we can enable it later with some testing. Your call.

Wireshark is happy with the generated frames, I can't test up to a 
running session but I'm sure it works.


Sylvain

signature.asc
Description: Digital signature