* [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain()
@ 2020-10-28 11:35 Lukas Bulwahn
2020-10-28 13:59 ` Tom Rix
2020-10-30 21:06 ` Jakub Kicinski
0 siblings, 2 replies; 5+ messages in thread
From: Lukas Bulwahn @ 2020-10-28 11:35 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S . Miller,
Jakub Kicinski, netdev
Cc: Nathan Chancellor, Nick Desaulniers, Tom Rix, clang-built-linux,
linux-kernel, kernel-janitors, linux-safety, Lukas Bulwahn
make clang-analyzer on x86_64 defconfig caught my attention with:
net/sched/cls_api.c:2964:3: warning: Value stored to 'parent' is never read
[clang-analyzer-deadcode.DeadStores]
parent = 0;
^
net/sched/cls_api.c:2977:4: warning: Value stored to 'parent' is never read
[clang-analyzer-deadcode.DeadStores]
parent = q->handle;
^
Commit 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
introduced tc_dump_chain() and this initial implementation already
contained these unneeded dead stores.
Simplify the code to make clang-analyzer happy.
As compilers will detect these unneeded assignments and optimize this
anyway, the resulting binary is identical before and after this change.
No functional change. No change in object code.
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on current master and next-20201028
Jamal, Cong, Jiri, please ack.
David, Jakub, please pick this minor non-urgent clean-up patch.
net/sched/cls_api.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index faeabff283a2..8ce830ca5f92 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2940,7 +2940,6 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
struct tcf_chain *chain;
long index_start;
long index;
- u32 parent;
int err;
if (nlmsg_len(cb->nlh) < sizeof(*tcm))
@@ -2955,13 +2954,6 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
block = tcf_block_refcnt_get(net, tcm->tcm_block_index);
if (!block)
goto out;
- /* If we work with block index, q is NULL and parent value
- * will never be used in the following code. The check
- * in tcf_fill_node prevents it. However, compiler does not
- * see that far, so set parent to zero to silence the warning
- * about parent being uninitialized.
- */
- parent = 0;
} else {
const struct Qdisc_class_ops *cops;
struct net_device *dev;
@@ -2971,13 +2963,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
if (!dev)
return skb->len;
- parent = tcm->tcm_parent;
- if (!parent) {
+ if (!tcm->tcm_parent)
q = dev->qdisc;
- parent = q->handle;
- } else {
+ else
q = qdisc_lookup(dev, TC_H_MAJ(tcm->tcm_parent));
- }
+
if (!q)
goto out;
cops = q->ops->cl_ops;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain()
2020-10-28 11:35 [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain() Lukas Bulwahn
@ 2020-10-28 13:59 ` Tom Rix
2020-10-28 15:50 ` Lukas Bulwahn
2020-10-28 17:38 ` Cong Wang
2020-10-30 21:06 ` Jakub Kicinski
1 sibling, 2 replies; 5+ messages in thread
From: Tom Rix @ 2020-10-28 13:59 UTC (permalink / raw)
To: Lukas Bulwahn, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
David S . Miller, Jakub Kicinski, netdev
Cc: Nathan Chancellor, Nick Desaulniers, clang-built-linux,
linux-kernel, kernel-janitors, linux-safety
On 10/28/20 4:35 AM, Lukas Bulwahn wrote:
> make clang-analyzer on x86_64 defconfig caught my attention with:
>
> net/sched/cls_api.c:2964:3: warning: Value stored to 'parent' is never read
> [clang-analyzer-deadcode.DeadStores]
> parent = 0;
> ^
>
> net/sched/cls_api.c:2977:4: warning: Value stored to 'parent' is never read
> [clang-analyzer-deadcode.DeadStores]
> parent = q->handle;
> ^
>
> Commit 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
> introduced tc_dump_chain() and this initial implementation already
> contained these unneeded dead stores.
>
> Simplify the code to make clang-analyzer happy.
>
> As compilers will detect these unneeded assignments and optimize this
> anyway, the resulting binary is identical before and after this change.
>
> No functional change. No change in object code.
>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> applies cleanly on current master and next-20201028
>
> Jamal, Cong, Jiri, please ack.
> David, Jakub, please pick this minor non-urgent clean-up patch.
>
> net/sched/cls_api.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index faeabff283a2..8ce830ca5f92 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -2940,7 +2940,6 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> struct tcf_chain *chain;
> long index_start;
> long index;
> - u32 parent;
> int err;
>
> if (nlmsg_len(cb->nlh) < sizeof(*tcm))
> @@ -2955,13 +2954,6 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> block = tcf_block_refcnt_get(net, tcm->tcm_block_index);
> if (!block)
> goto out;
> - /* If we work with block index, q is NULL and parent value
> - * will never be used in the following code. The check
> - * in tcf_fill_node prevents it. However, compiler does not
> - * see that far, so set parent to zero to silence the warning
> - * about parent being uninitialized.
> - */
> - parent = 0;
> } else {
> const struct Qdisc_class_ops *cops;
> struct net_device *dev;
> @@ -2971,13 +2963,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> if (!dev)
> return skb->len;
>
> - parent = tcm->tcm_parent;
> - if (!parent) {
> + if (!tcm->tcm_parent)
> q = dev->qdisc;
> - parent = q->handle;
This looks like a an unused error handler.
and the later call to
if (TC_H_MIN(tcm->tcm_parent)
maybe should be
if (TC_H_MIN(parent))
so I am skeptical that this change is ok because the code around it looks buggy.
Tom
> - } else {
> + else
> q = qdisc_lookup(dev, TC_H_MAJ(tcm->tcm_parent));
> - }
> +
> if (!q)
> goto out;
> cops = q->ops->cl_ops;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain()
2020-10-28 13:59 ` Tom Rix
@ 2020-10-28 15:50 ` Lukas Bulwahn
2020-10-28 17:38 ` Cong Wang
1 sibling, 0 replies; 5+ messages in thread
From: Lukas Bulwahn @ 2020-10-28 15:50 UTC (permalink / raw)
To: Tom Rix
Cc: Lukas Bulwahn, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
David S . Miller, Jakub Kicinski, netdev, Nathan Chancellor,
Nick Desaulniers, clang-built-linux, linux-kernel,
kernel-janitors, linux-safety
On Wed, 28 Oct 2020, Tom Rix wrote:
>
> On 10/28/20 4:35 AM, Lukas Bulwahn wrote:
> > make clang-analyzer on x86_64 defconfig caught my attention with:
> >
> > net/sched/cls_api.c:2964:3: warning: Value stored to 'parent' is never read
> > [clang-analyzer-deadcode.DeadStores]
> > parent = 0;
> > ^
> >
> > net/sched/cls_api.c:2977:4: warning: Value stored to 'parent' is never read
> > [clang-analyzer-deadcode.DeadStores]
> > parent = q->handle;
> > ^
> >
> > Commit 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
> > introduced tc_dump_chain() and this initial implementation already
> > contained these unneeded dead stores.
> >
> > Simplify the code to make clang-analyzer happy.
> >
> > As compilers will detect these unneeded assignments and optimize this
> > anyway, the resulting binary is identical before and after this change.
> >
> > No functional change. No change in object code.
> >
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> > applies cleanly on current master and next-20201028
> >
> > Jamal, Cong, Jiri, please ack.
> > David, Jakub, please pick this minor non-urgent clean-up patch.
> >
> > net/sched/cls_api.c | 16 +++-------------
> > 1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index faeabff283a2..8ce830ca5f92 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -2940,7 +2940,6 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> > struct tcf_chain *chain;
> > long index_start;
> > long index;
> > - u32 parent;
> > int err;
> >
> > if (nlmsg_len(cb->nlh) < sizeof(*tcm))
> > @@ -2955,13 +2954,6 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> > block = tcf_block_refcnt_get(net, tcm->tcm_block_index);
> > if (!block)
> > goto out;
> > - /* If we work with block index, q is NULL and parent value
> > - * will never be used in the following code. The check
> > - * in tcf_fill_node prevents it. However, compiler does not
> > - * see that far, so set parent to zero to silence the warning
> > - * about parent being uninitialized.
> > - */
> > - parent = 0;
> > } else {
> > const struct Qdisc_class_ops *cops;
> > struct net_device *dev;
> > @@ -2971,13 +2963,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> > if (!dev)
> > return skb->len;
> >
> > - parent = tcm->tcm_parent;
> > - if (!parent) {
> > + if (!tcm->tcm_parent)
> > q = dev->qdisc;
> > - parent = q->handle;
>
> This looks like a an unused error handler.
>
> and the later call to
>
> if (TC_H_MIN(tcm->tcm_parent)
>
> maybe should be
>
> if (TC_H_MIN(parent))
>
> so I am skeptical that this change is ok because the code around it looks buggy.
>
Maybe that is the case.
Certainly the comment above about being uninitialized is outdated as
parent is not used in tc_chain_fill_node().
I had another look and I noticed a copy of this same pattern (with the
same comment) in tc_dump_tfilter(), but it seems that the two copies have
somehow diverged over time. Certainly, something is fishy here.
I guess it needs some more digging in the code...
Lukas
> Tom
>
> > - } else {
> > + else
> > q = qdisc_lookup(dev, TC_H_MAJ(tcm->tcm_parent));
> > - }
> > +
> > if (!q)
> > goto out;
> > cops = q->ops->cl_ops;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain()
2020-10-28 13:59 ` Tom Rix
2020-10-28 15:50 ` Lukas Bulwahn
@ 2020-10-28 17:38 ` Cong Wang
1 sibling, 0 replies; 5+ messages in thread
From: Cong Wang @ 2020-10-28 17:38 UTC (permalink / raw)
To: Tom Rix
Cc: Lukas Bulwahn, Jamal Hadi Salim, Jiri Pirko, David S . Miller,
Jakub Kicinski, Linux Kernel Network Developers,
Nathan Chancellor, Nick Desaulniers, clang-built-linux, LKML,
kernel-janitors, linux-safety
On Wed, Oct 28, 2020 at 6:59 AM Tom Rix <trix@redhat.com> wrote:
>
>
> On 10/28/20 4:35 AM, Lukas Bulwahn wrote:
> > @@ -2971,13 +2963,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> > if (!dev)
> > return skb->len;
> >
> > - parent = tcm->tcm_parent;
> > - if (!parent) {
> > + if (!tcm->tcm_parent)
> > q = dev->qdisc;
> > - parent = q->handle;
>
> This looks like a an unused error handler.
>
> and the later call to
>
> if (TC_H_MIN(tcm->tcm_parent)
>
> maybe should be
>
> if (TC_H_MIN(parent))
When tcm->tcm_parent is 0, TC_H_MIN(tcm->tcm_parent) is also 0,
so we will not hit that if branch.
So, I think Lukas' patch is correct.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain()
2020-10-28 11:35 [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain() Lukas Bulwahn
2020-10-28 13:59 ` Tom Rix
@ 2020-10-30 21:06 ` Jakub Kicinski
1 sibling, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2020-10-30 21:06 UTC (permalink / raw)
To: Lukas Bulwahn
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S . Miller,
netdev, Nathan Chancellor, Nick Desaulniers, Tom Rix,
clang-built-linux, linux-kernel, kernel-janitors, linux-safety
On Wed, 28 Oct 2020 12:35:33 +0100 Lukas Bulwahn wrote:
> make clang-analyzer on x86_64 defconfig caught my attention with:
>
> net/sched/cls_api.c:2964:3: warning: Value stored to 'parent' is never read
> [clang-analyzer-deadcode.DeadStores]
> parent = 0;
> ^
>
> net/sched/cls_api.c:2977:4: warning: Value stored to 'parent' is never read
> [clang-analyzer-deadcode.DeadStores]
> parent = q->handle;
> ^
>
> Commit 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
> introduced tc_dump_chain() and this initial implementation already
> contained these unneeded dead stores.
>
> Simplify the code to make clang-analyzer happy.
>
> As compilers will detect these unneeded assignments and optimize this
> anyway, the resulting binary is identical before and after this change.
>
> No functional change. No change in object code.
>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> applies cleanly on current master and next-20201028
>
> Jamal, Cong, Jiri, please ack.
> David, Jakub, please pick this minor non-urgent clean-up patch.
Applied, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-30 21:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28 11:35 [PATCH] net: cls_api: remove unneeded local variable in tc_dump_chain() Lukas Bulwahn
2020-10-28 13:59 ` Tom Rix
2020-10-28 15:50 ` Lukas Bulwahn
2020-10-28 17:38 ` Cong Wang
2020-10-30 21:06 ` Jakub Kicinski
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 NNTP newsgroup(s).