Re: [lwip-devel] IPv6 L2TP PPP review

[Ivan Delamer]

Hello Sylvain,

I think your code is correct. I use something very similar for UDP 
connections, only using netconns.

In general once you are working with UDP or TCP, it is really quite 
similar for IPv4 or IPv6, because you are one layer over that. It is 
just address binding/comparison that needs to be done appropriately.

The API is not completely safe and requires care from the programmer.

I think we have it as a task to improve ipX_addr_t to include a version 
field. We just wanted to initially implement IPv6 in a way that was easy 
to turn it off, and the resulting stack was exactly the same as the old 
IPv4-only stack. We will have to improve it though...

Cheers
Ivan


On 2015-03-02 13:02, Sylvain Rochet wrote:
> 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