Re: [PATCH net v2 1/2] ipv6: Dump route exceptions too in rt6_dump_route()

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 8 Jun 2019 06:15:51 +0000
Martin Lau <kafai@fb.com> wrote:

> > @@ -473,12 +473,22 @@ static int fib6_dump_node(struct fib6_walker *w)
> >  	struct fib6_info *rt;
> >  
> >  	for_each_fib6_walker_rt(w) {
> > -		res = rt6_dump_route(rt, w->args);
> > -		if (res < 0) {
> > +		res = rt6_dump_route(rt, w->args, w->skip_in_node);
> > +		if (res) {
> >  			/* Frame is full, suspend walking */
> >  			w->leaf = rt;
> > +
> > +			/* We'll restart from this node, so if some routes were
> > +			 * already dumped, skip them next time.
> > +			 */
> > +			if (res > 0)
> > +				w->skip_in_node += res;
> > +			else
> > +				w->skip_in_node = 0;  
> I am likely missing something.  It is not obvious to me why skip_in_node
> can go backward to 0 here when res < 0.

I'm not taking into account the case where we initially manage to dump
routes, and on a second attempt the buffer is smaller so we can't dump
any, so here I considered that -1 would only happen the first time we
hit a given node.

> Should skip_in_node be strictly increasing to ensure forward progress?

Yes, I guess that would be more robust. I'll change that.

> Would it be more intuitive to change the return value of
> rt6_dump_route() such that
> -1: done with this node
> >=0: number of routes filled in this round but still some more to be done?  
> 
> then:
> if (res >= 0) {
> 	w->leaf = rt;
> 	w->skip_in_node += res;
> 	return 1;
> }

Hm, maybe, I don't really have a preference. Returning 0 on success
looked more canonical, but your version is a bit more terse after all.
Sure, I can turn it that way.

> > @@ -4871,20 +4875,69 @@ int rt6_dump_route(struct fib6_info *rt, void *p_arg)
> >  	if ((filter->flags & RTM_F_PREFIX) &&
> >  	    !(rt->fib6_flags & RTF_PREFIX_RT)) {
> >  		/* success since this is not a prefix route */
> > -		return 1;
> > +		return 0;
> >  	}
> >  	if (filter->filter_set) {
> >  		if ((filter->rt_type && rt->fib6_type != filter->rt_type) ||
> >  		    (filter->dev && !fib6_info_uses_dev(rt, filter->dev)) ||
> >  		    (filter->protocol && rt->fib6_protocol != filter->protocol)) {
> > -			return 1;
> > +			return 0;
> >  		}
> >  		flags |= NLM_F_DUMP_FILTERED;
> >  	}
> >  
> > -	return rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL, 0,
> > -			     RTM_NEWROUTE, NETLINK_CB(arg->cb->skb).portid,
> > -			     arg->cb->nlh->nlmsg_seq, flags);
> > +	if (!(filter->flags & RTM_F_CLONED)) {
> > +		if (skip) {
> > +			skip--;
> > +		} else if (rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL,
> > +					 0, RTM_NEWROUTE,
> > +					 NETLINK_CB(arg->cb->skb).portid,
> > +					 arg->cb->nlh->nlmsg_seq, flags)) {
> > +			return -1;
> > +		} else {  
> If the v1 email thread will be concluded to dump exceptions only when cloned
> flag is set, it may need some changes in this function.

Indeed, it would also look less ugly (skip_in_node is only for
exceptions at that point).

-- 
Stefano