Message ID | 20200418013735.67882-1-maowenan@huawei.com |
---|---|
State | Accepted |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next,v2] bpf: remove set but not used variable 'dst_known' | expand |
> On Apr 17, 2020, at 6:37 PM, Mao Wenan <maowenan@huawei.com> wrote: > > Fixes gcc '-Wunused-but-set-variable' warning: > > kernel/bpf/verifier.c:5603:18: warning: variable ‘dst_known’ > set but not used [-Wunused-but-set-variable], delete this > variable. > > Signed-off-by: Mao Wenan <maowenan@huawei.com> Acked-by: Song Liu <songliubraving@fb.com> With one nit below. > --- > v2: remove fixes tag in commit log. > kernel/bpf/verifier.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 04c6630cc18f..c9f50969a689 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5600,7 +5600,7 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, > { > struct bpf_reg_state *regs = cur_regs(env); > u8 opcode = BPF_OP(insn->code); > - bool src_known, dst_known; > + bool src_known; This is not a hard rule, but we prefer to keep variable definition in "reverse Christmas tree" order. Since we are on this function, let's reorder these definitions to something like: u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32; struct bpf_reg_state *regs = cur_regs(env); u8 opcode = BPF_OP(insn->code); u32 dst = insn->dst_reg; s64 smin_val, smax_val; u64 umin_val, umax_val; bool src_known; int ret;
On Sat, Apr 18, 2020 at 06:13:48AM +0000, Song Liu wrote: > > > > On Apr 17, 2020, at 6:37 PM, Mao Wenan <maowenan@huawei.com> wrote: > > > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > kernel/bpf/verifier.c:5603:18: warning: variable ‘dst_known’ > > set but not used [-Wunused-but-set-variable], delete this > > variable. > > > > Signed-off-by: Mao Wenan <maowenan@huawei.com> > > Acked-by: Song Liu <songliubraving@fb.com> > > With one nit below. > > > --- > > v2: remove fixes tag in commit log. > > kernel/bpf/verifier.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 04c6630cc18f..c9f50969a689 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -5600,7 +5600,7 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, > > { > > struct bpf_reg_state *regs = cur_regs(env); > > u8 opcode = BPF_OP(insn->code); > > - bool src_known, dst_known; > > + bool src_known; > > This is not a hard rule, but we prefer to keep variable definition in > "reverse Christmas tree" order. Since we are on this function, let's > reorder these definitions to something like: > > u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32; > struct bpf_reg_state *regs = cur_regs(env); > u8 opcode = BPF_OP(insn->code); > u32 dst = insn->dst_reg; > s64 smin_val, smax_val; > u64 umin_val, umax_val; > bool src_known; > int ret; I don't want folks to keep re-sorting variables and making patches difficult to backport, do git blame, causing bpf vs bpf-next conflicts, etc. reverse xmas tree is not mandatory. It's a style preference. I personally do it for new code, but very rarely for fixes. And certainly not for this kind of cleanup. Applied. Thanks
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 04c6630cc18f..c9f50969a689 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5600,7 +5600,7 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, { struct bpf_reg_state *regs = cur_regs(env); u8 opcode = BPF_OP(insn->code); - bool src_known, dst_known; + bool src_known; s64 smin_val, smax_val; u64 umin_val, umax_val; s32 s32_min_val, s32_max_val; @@ -5622,7 +5622,6 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, if (alu32) { src_known = tnum_subreg_is_const(src_reg.var_off); - dst_known = tnum_subreg_is_const(dst_reg->var_off); if ((src_known && (s32_min_val != s32_max_val || u32_min_val != u32_max_val)) || s32_min_val > s32_max_val || u32_min_val > u32_max_val) { @@ -5634,7 +5633,6 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, } } else { src_known = tnum_is_const(src_reg.var_off); - dst_known = tnum_is_const(dst_reg->var_off); if ((src_known && (smin_val != smax_val || umin_val != umax_val)) || smin_val > smax_val || umin_val > umax_val) {
Fixes gcc '-Wunused-but-set-variable' warning: kernel/bpf/verifier.c:5603:18: warning: variable ‘dst_known’ set but not used [-Wunused-but-set-variable], delete this variable. Signed-off-by: Mao Wenan <maowenan@huawei.com> --- v2: remove fixes tag in commit log. kernel/bpf/verifier.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)