Re: [PATCH mptcp-next v2 7/7] selftests: mptcp: flush userspace addrs list

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/02/2024 09:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper userspace_pm_flush() to flush all addresses
> for the userspace PM. Invoke it in userspace pm dump address and subflow
> tests. And use dump commands to check if the userspace pm local address
> list is empty after addresses flushing.

I'm sorry to insist, but please *always* *always* *always* add an answer
to this question in your commit message: why should we take this patch?

Answers can be very short -- e.g. avoid duplicated code, fix a bug when
doing X, this new feature is useful for this use-case, etc. -- but these
answers *have to* be present.

Here, the commit message only explains what is being done: that's
interesting to verify if what you wanted to do is what you did. But most
of the time, we can see what is being done by quickly looking at the
diff. What is *not easy* to get by looking at the diff is the *reason*
why this patch is needed.

Of course, ↑ is valid for any patches for the Linux kernel, as mentioned
in different places in the doc:
-
https://docs.kernel.org/process/submitting-patches.html?highlight=reason#the-canonical-patch-format

> The explanation body will be committed to the permanent source changelog, so should make sense to a competent reader who has long since forgotten the immediate details of the discussion that might have led to this patch. Including symptoms of the failure which the patch addresses (kernel log messages, oops messages, etc.) are especially useful for people who might be searching the commit logs looking for the applicable patch. The text should be written in such detail so that when read weeks, months or even years later, it can give the reader the needed details to grasp the reasoning for why the patch was created.

-
https://docs.kernel.org/process/maintainer-netdev.html?highlight=reason#preparing-changes

> If your change is a bug fix, make sure your commit log indicates the end-user visible symptom, the underlying reason as to why it happens, and then if necessary, explain why the fix proposed is the best way to get things done.



Now back to this patch, I can very quickly see that you added a new
helper, called it, and checked there were no more addresses. But without
the reason in the commit message, I don't understand why we should do that:
- the output of userspace_pm_dump() is already verified
- the actions done by userspace_pm_rm_addr() and userspace_pm_rm_sf()
helpers are also already verified.
- it looks like we are not covering more .c code by doing that, no?
→ What are you trying to verify that was not verified before?

Maybe we don't need the patch?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.