On 08/31/2016 04:51 PM, Nicolas Dichtel wrote:
> Thanks for proposing this feature. It would be great to have it upstream.
> 

Thanks for the feedback :)

> [snip]
>> +config IPV6_SEG6
>> +    bool "IPv6: Segment Routing support"
>> +    depends on IPV6
>> +    ---help---
>> +      Experimental support for IPv6 Segment Routing dataplane as defined
>> +      in IETF draft-ietf-6man-segment-routing-header-01. This option
>> +      enables the processing of SR-enabled packets allowing the kernel
>> +      to act as a segment endpoint (intermediate or egress).
>> +
>> +      If unsure, say N.
>> +
> I don't think that the option is needed. At the end, every distributions will
> turn it on.
> 

Are you sure ? This is a rather specific feature, used in specific
environments. Not that I would mind removing the option if it makes sense.

> [snip]
>> +#ifdef CONFIG_IPV6_SEG6
>> +    {
>> +            .procname       = "seg6_enabled",
>> +            .data           = &ipv6_devconf.seg6_enabled,
>> +            .maxlen         = sizeof(int),
>> +            .mode           = 0644,
>> +            .proc_handler   = proc_dointvec,
>> +    },
>> +#endif
> Don't forget to document this option in 
> Documentation/networking/ip-sysctl.txt.
> Don't forget to explain how 'all' works ;-)
> It would be nice to also add it in netconf subsystem (see 'git grep netconf
> net/ipv6').
> 

Right ! I didn't think of that doc file. Noted for netconf.

> [snip]
>> +#ifdef CONFIG_IPV6_SEG6
>> +static int ipv6_srh_rcv(struct sk_buff *skb)
>> +{
>> +    struct inet6_skb_parm *opt = IP6CB(skb);
>> +    struct in6_addr *addr = NULL, *last_addr = NULL, *active_addr = NULL;
>> +    struct ipv6_sr_hdr *hdr;
>> +    struct net *net = dev_net(skb->dev);
>> +    int cleanup = 0;
>> +    struct inet6_dev *idev;
>> +    int accept_seg6;
> nit: better to follow the 'reverse christmas tree' scheme when declaring 
> variables.
> 

Noted


>> +
>> +    ip6_route_input(skb);
> The destination address has now changed and the packet is routed again.
> skb->nfct is not updated, it is intentional? I'm asking me if it's 
> conceptually
> right.
> 

I fail to see any usecase where conntrack would run on SR-enabled
packets. Things such as NAT would just defeat the purpose.

David

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to