Re: [PATCH mptcp-next v6 3/7] mptcp: pm: improve error messages

[Matthieu Baerts <matttbe@kernel.org>]

On 06/01/2025 09:55, Geliang Tang wrote:
> Happy New Year Matt!

Thank you! :)

Happy New Year as well (even if I should probably say that to you at the
end of January :) )

> On Mon, 2025-01-06 at 09:45 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 06/01/2025 09:41, Geliang Tang wrote:
>>> On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
>>>> Some error messages were:
>>>>
>>>>  - too generic: "missing input", "invalid request"
>>>>
>>>>  - not precise enough: "limit greater than maximum" but what's
>>>> the
>>>> max?
>>>>
>>>>  - missing: subflow not found, or connect error.
>>>>
>>>> This can be easily improved by being more precise, or adding new
>>>> error
>>>> messages.
>>
>> (...)
>>
>>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>>>> index
>>>> 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998
>>>> cf76
>>>> 46368f8201e2e 100644
>>>> --- a/net/mptcp/pm_userspace.c
>>>> +++ b/net/mptcp/pm_userspace.c
>>
>> (...)
>>
>>>> @@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct
>>>> sk_buff
>>>> *skb, struct genl_info *info)
>>>>  	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr,
>>>> &rem.addr, bkup);
>>>>  	release_sock(sk);
>>>>  
>>>> +	if (ret)
>>>> +		GENL_SET_ERR_MSG(info, "subflow not found");
>>>
>>> I guess you don't want to use "subflow not found" here. I changed
>>> it as
>>> "mp_prio send ack failed" in v7.
>>
>> No, I wanted to use "subflow not found":
>> mptcp_pm_nl_mp_prio_send_ack()
>> will fail only if it cannot find the subflow matching the local and
>> remote attributes.
> 
> My bad, how about using "subflow not found in set_flags" here, and use
> "subflow not found in subflow_destroy" for
> mptcp_pm_nl_subflow_destroy_doit?

I don't think we need a different error message: the userspace will send
a specific Netlink command, and an error can be returned. No need to
mention which command it is I think.

If you prefer, feel free to add a comment above this error here:

/* mptcp_pm_nl_mp_prio_send_ack will fail if it cannot find a subflow */

While at it, if you plan to send a v8, please use 'if (ret < 0)' to
clearly show we are looking at errors. (When the variable is called
'err', that's clearer, but no need to change it here.)

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