| Message ID | 2274e7768f2534184f675b86b61c2f434290f98a.1729588019.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | BPF path manager | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/build | warning | Build error with: make C=1 net/mptcp/bpf.o |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
Hi Geliang, On 22/10/2024 11:14, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The helper mptcp_pm_addr_families_match() uses "if-else" to handle IPv6 > and IPv4 addresses separately. But the last line of "if" code block is > a "return" statement. In this case, no need to use an "else" statement. > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > net/mptcp/pm.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c > index 16c336c51940..f3d354a72c94 100644 > --- a/net/mptcp/pm.c > +++ b/net/mptcp/pm.c > @@ -495,9 +495,8 @@ bool mptcp_pm_addr_families_match(const struct sock *sk, > return !loc_is_v4 && !rem_is_v4; > > return loc_is_v4 == rem_is_v4; > -#else > - return mptcp_is_v4 && loc->family == AF_INET && rem->family == AF_INET; > #endif > + return mptcp_is_v4 && loc->family == AF_INET && rem->family == AF_INET; I think some static analytic tools will complain because if CONFIG_MPTCP_IPV6 is enabled, the code will look like this: return loc_is_v4 == rem_is_v4; return mptcp_is_v4 && (...) Two 'return' in a row, the 2nd return is never used, there will be a warning somewhere. Also, I don't it is worth it, and it looks clearer with the #else. Is it OK to drop this patch when applying the series? > } > > void mptcp_pm_data_reset(struct mptcp_sock *msk) Cheers, Matt
Hi Matt, Thanks for your review. On Tue, 2024-10-22 at 19:01 +0200, Matthieu Baerts wrote: > Hi Geliang, > > On 22/10/2024 11:14, Geliang Tang wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > The helper mptcp_pm_addr_families_match() uses "if-else" to handle > > IPv6 > > and IPv4 addresses separately. But the last line of "if" code block > > is > > a "return" statement. In this case, no need to use an "else" > > statement. > > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > > --- > > net/mptcp/pm.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c > > index 16c336c51940..f3d354a72c94 100644 > > --- a/net/mptcp/pm.c > > +++ b/net/mptcp/pm.c > > @@ -495,9 +495,8 @@ bool mptcp_pm_addr_families_match(const struct > > sock *sk, > > return !loc_is_v4 && !rem_is_v4; > > > > return loc_is_v4 == rem_is_v4; > > -#else > > - return mptcp_is_v4 && loc->family == AF_INET && rem- > > >family == AF_INET; > > #endif > > + return mptcp_is_v4 && loc->family == AF_INET && rem- > > >family == AF_INET; > > I think some static analytic tools will complain because if > CONFIG_MPTCP_IPV6 is enabled, the code will look like this: > > return loc_is_v4 == rem_is_v4; > return mptcp_is_v4 && (...) > > Two 'return' in a row, the 2nd return is never used, there will be a > warning somewhere. > > Also, I don't it is worth it, and it looks clearer with the #else. Is > it > OK to drop this patch when applying the series? Let's drop it then. It has nothing to do with the entire BPF path manager set, and other patches have no dependencies on it too. Thanks, -Geliang > > > } > > > > void mptcp_pm_data_reset(struct mptcp_sock *msk) > > Cheers, > Matt
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 16c336c51940..f3d354a72c94 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -495,9 +495,8 @@ bool mptcp_pm_addr_families_match(const struct sock *sk, return !loc_is_v4 && !rem_is_v4; return loc_is_v4 == rem_is_v4; -#else - return mptcp_is_v4 && loc->family == AF_INET && rem->family == AF_INET; #endif + return mptcp_is_v4 && loc->family == AF_INET && rem->family == AF_INET; } void mptcp_pm_data_reset(struct mptcp_sock *msk)