Let's CC Felix on this one because he might know the answer.
All day long I spend looking at code like this:
net/core/dev.c:724 dev_fill_forward_path() info: returning a literal zero is cleaner
net/core/dev.c:732 dev_fill_forward_path() info: returning a literal zero is cleaner
net/core/dev.c
696 int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
697 struct net_device_path_stack *stack)
698 {
699 const struct net_device *last_dev;
700 struct net_device_path_ctx ctx = {
701 .dev = dev,
702 };
703 struct net_device_path *path;
704 int ret = 0;
705
706 memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
707 stack->num_paths = 0;
708 while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
709 last_dev = ctx.dev;
710 path = dev_fwd_path(stack);
711 if (!path)
712 return -1;
713
714 memset(path, 0, sizeof(struct net_device_path));
715 ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
716 if (ret < 0)
This if condition might trick you into thinking that ->ndo_fill_forward_path()
can return non-zero positive numbers, but it can't. It returns zero on
success or negative error codes on failure. Smatch is doing cross
function analysis so we know this.
717 return -1;
718
719 if (WARN_ON_ONCE(last_dev == ctx.dev))
720 return -1;
721 }
722
723 if (!ctx.dev)
724 return ret;
Is this intentional or not? Who knows? If this were an obvious bug,
I could fix it right away but ambiguous stuff like this takes way more
time to deal with.
725
726 path = dev_fwd_path(stack);
727 if (!path)
728 return -1;
729 path->type = DEV_PATH_ETHERNET;
730 path->dev = ctx.dev;
731
732 return ret;
Obviously this is intentional, but if you were tricked by the checking
earlier then you might assume that ret is some positive value from the
last iteration through the loop. "return 0;" is so much clearer.
733 }
regards,
dan carpetner
