On Sat, 8 Jun 2019 06:15:51 +0000
Martin Lau <ka...@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
