| Message ID | 20250207-net-next-mptcp-pm-misc-cleanup-2-v3-1-71753ed957de@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: pm: misc cleanups, part 2 | expand |
On Fri, Feb 07, 2025 at 02:59:19PM +0100, Matthieu Baerts (NGI0) wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The only use of 'info' parameter of userspace_pm_remove_id_zero_address() > is to set an error message into it. > > Plus, this helper will only fail when it cannot find any subflows with a > local address ID 0. > > This patch drops this parameter and sets the error message where this > function is called in mptcp_pm_nl_remove_doit(). > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> Reviewed-by: Simon Horman <horms@kernel.org> Hi Mat, A minor nit, perhaps it has been discussed before: I'm not sure that your Reviewed-by is needed if you also provide your Signed-off-by. Because it I think that the latter implies the former.
Hi Simon, On 10/02/2025 20:49, Simon Horman wrote: > On Fri, Feb 07, 2025 at 02:59:19PM +0100, Matthieu Baerts (NGI0) wrote: >> From: Geliang Tang <tanggeliang@kylinos.cn> >> >> The only use of 'info' parameter of userspace_pm_remove_id_zero_address() >> is to set an error message into it. >> >> Plus, this helper will only fail when it cannot find any subflows with a >> local address ID 0. >> >> This patch drops this parameter and sets the error message where this >> function is called in mptcp_pm_nl_remove_doit(). >> >> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> >> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > Reviewed-by: Simon Horman <horms@kernel.org> Thank you for the review, and this message! > A minor nit, perhaps it has been discussed before: > > I'm not sure that your Reviewed-by is needed if you also provide > your Signed-off-by. Because it I think that the latter implies the former. This has been discussed a while ago, but only on the MPTCP list I think. To be honest, we didn't find a precise answer in the doc [1], and maybe we are doing it wrong for all this time :) Technically, when someone shares a patch on the MPTCP ML, someone else does the review, sent the "Reviewed-by" tag, then the patch is queued, and the one sending the patch to the netdev ML adds a "Signed-off-by" tag. With this patch here, I did both. Before, we were removing the RvB tag when it was the same as the SoB one, but we stopped doing that because we thought that was not correct and / or not needed. We can re-introduce this if preferred. My understanding is that the SoB tag is for the authors and the intermediate maintainers -- who might have not done a full review -- while the RvB one seems to indicate that a "proper" review has been done. If someone else does a review on a patch, I can add my SoB tag when "forwarding" the patch, trusting the review done by someone else. Do you think it is better to remove the RvB tag if there is a SoB one for the same person? [1] https://docs.kernel.org/process/submitting-patches.html Cheers, Matt
On Tue, Feb 11, 2025 at 10:31:05AM +0100, Matthieu Baerts wrote: > Hi Simon, > > On 10/02/2025 20:49, Simon Horman wrote: > > On Fri, Feb 07, 2025 at 02:59:19PM +0100, Matthieu Baerts (NGI0) wrote: > >> From: Geliang Tang <tanggeliang@kylinos.cn> > >> > >> The only use of 'info' parameter of userspace_pm_remove_id_zero_address() > >> is to set an error message into it. > >> > >> Plus, this helper will only fail when it cannot find any subflows with a > >> local address ID 0. > >> > >> This patch drops this parameter and sets the error message where this > >> function is called in mptcp_pm_nl_remove_doit(). > >> > >> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > >> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > Thank you for the review, and this message! > > > A minor nit, perhaps it has been discussed before: > > > > I'm not sure that your Reviewed-by is needed if you also provide > > your Signed-off-by. Because it I think that the latter implies the former. > > This has been discussed a while ago, but only on the MPTCP list I think. > To be honest, we didn't find a precise answer in the doc [1], and maybe > we are doing it wrong for all this time :) > > Technically, when someone shares a patch on the MPTCP ML, someone else > does the review, sent the "Reviewed-by" tag, then the patch is queued, > and the one sending the patch to the netdev ML adds a "Signed-off-by" > tag. With this patch here, I did both. > > Before, we were removing the RvB tag when it was the same as the SoB > one, but we stopped doing that because we thought that was not correct > and / or not needed. We can re-introduce this if preferred. My > understanding is that the SoB tag is for the authors and the > intermediate maintainers -- who might have not done a full review -- > while the RvB one seems to indicate that a "proper" review has been > done. If someone else does a review on a patch, I can add my SoB tag > when "forwarding" the patch, trusting the review done by someone else. > > Do you think it is better to remove the RvB tag if there is a SoB one > for the same person? > > [1] https://docs.kernel.org/process/submitting-patches.html Hi Mat, Thanks for the explanation. I see that in your process the Reviewed-by and Signed-off-by have distinct meanings. Which does make sense. I'm ambivalent regarding which way to go (sorry that isn't very helpful). But I do suspect I won't be the last person to ask about this.
On 11/02/2025 11:13, Simon Horman wrote: > On Tue, Feb 11, 2025 at 10:31:05AM +0100, Matthieu Baerts wrote: >> Hi Simon, >> >> On 10/02/2025 20:49, Simon Horman wrote: >>> On Fri, Feb 07, 2025 at 02:59:19PM +0100, Matthieu Baerts (NGI0) wrote: >>>> From: Geliang Tang <tanggeliang@kylinos.cn> >>>> >>>> The only use of 'info' parameter of userspace_pm_remove_id_zero_address() >>>> is to set an error message into it. >>>> >>>> Plus, this helper will only fail when it cannot find any subflows with a >>>> local address ID 0. >>>> >>>> This patch drops this parameter and sets the error message where this >>>> function is called in mptcp_pm_nl_remove_doit(). >>>> >>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> >>>> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >>> >>> Reviewed-by: Simon Horman <horms@kernel.org> >> >> Thank you for the review, and this message! >> >>> A minor nit, perhaps it has been discussed before: >>> >>> I'm not sure that your Reviewed-by is needed if you also provide >>> your Signed-off-by. Because it I think that the latter implies the former. >> >> This has been discussed a while ago, but only on the MPTCP list I think. >> To be honest, we didn't find a precise answer in the doc [1], and maybe >> we are doing it wrong for all this time :) >> >> Technically, when someone shares a patch on the MPTCP ML, someone else >> does the review, sent the "Reviewed-by" tag, then the patch is queued, >> and the one sending the patch to the netdev ML adds a "Signed-off-by" >> tag. With this patch here, I did both. >> >> Before, we were removing the RvB tag when it was the same as the SoB >> one, but we stopped doing that because we thought that was not correct >> and / or not needed. We can re-introduce this if preferred. My >> understanding is that the SoB tag is for the authors and the >> intermediate maintainers -- who might have not done a full review -- >> while the RvB one seems to indicate that a "proper" review has been >> done. If someone else does a review on a patch, I can add my SoB tag >> when "forwarding" the patch, trusting the review done by someone else. >> >> Do you think it is better to remove the RvB tag if there is a SoB one >> for the same person? >> >> [1] https://docs.kernel.org/process/submitting-patches.html > > Hi Mat, > > Thanks for the explanation. I see that in your process the Reviewed-by > and Signed-off-by have distinct meanings. Which does make sense. > > I'm ambivalent regarding which way to go (sorry that isn't very helpful). > But I do suspect I won't be the last person to ask about this. That's OK, now I have a canned reply ready to be sent for that :) Cheers, Matt
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c index a3d477059b11c3a5618dbb6256434a8e55845995..4de38bc03ab8add367720262f353dd20cacac108 100644 --- a/net/mptcp/pm_userspace.c +++ b/net/mptcp/pm_userspace.c @@ -253,8 +253,7 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info) return err; } -static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk, - struct genl_info *info) +static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk) { struct mptcp_rm_list list = { .nr = 0 }; struct mptcp_subflow_context *subflow; @@ -269,10 +268,8 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk, break; } } - if (!has_id_0) { - GENL_SET_ERR_MSG(info, "address with id 0 not found"); + if (!has_id_0) goto remove_err; - } list.ids[list.nr++] = 0; @@ -330,7 +327,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) sk = (struct sock *)msk; if (id_val == 0) { - err = mptcp_userspace_pm_remove_id_zero_address(msk, info); + err = mptcp_userspace_pm_remove_id_zero_address(msk); goto out; } @@ -339,7 +336,6 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) spin_lock_bh(&msk->pm.lock); match = mptcp_userspace_pm_lookup_addr_by_id(msk, id_val); if (!match) { - GENL_SET_ERR_MSG(info, "address with specified id not found"); spin_unlock_bh(&msk->pm.lock); release_sock(sk); goto out; @@ -356,6 +352,11 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) err = 0; out: + if (err) + GENL_SET_ERR_MSG_FMT(info, + "address with id %u not found", + id_val); + sock_put(sk); return err; }