From: Dan Carpenter <dan.carpenter@linaro.org>
To: Danilo Krummrich <dakr@redhat.com>, Felix Fietkau <nbd@nbd.name>
Cc: nouveau@lists.freedesktop.org, kernel-janitors@vger.kernel.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Daniel Vetter <daniel@ffwll.ch>,
Colin Ian King <colin.i.king@gmail.com>
Subject: Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
Date: Tue, 23 Jan 2024 11:56:49 +0300 [thread overview]
Message-ID: <fe659736-500f-4e59-a9c7-ad894155a675@moroto.mountain> (raw)
In-Reply-To: <55f0983a-300e-40bb-9142-6f4683914402@moroto.mountain>
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
prev parent reply other threads:[~2024-01-23 8:57 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-16 11:16 [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret Colin Ian King
2024-01-16 12:31 ` Dan Carpenter
2024-01-22 23:04 ` Danilo Krummrich
2024-01-23 7:38 ` Dan Carpenter
2024-01-23 8:56 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fe659736-500f-4e59-a9c7-ad894155a675@moroto.mountain \
--to=dan.carpenter@linaro.org \
--cc=colin.i.king@gmail.com \
--cc=dakr@redhat.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nbd@nbd.name \
--cc=nouveau@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).