Re: [PATCH mptcp-next v6 2/7] mptcp: pm: more precise error messages

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/01/2025 09:38, Geliang Tang wrote:
> On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
>> Some errors reported by the userspace PM were vague: "this or that is
>> invalid".
>>
>> It is easier for the userspace to know which part is wrong, instead
>> of
>> having to guess that.
>>
>> By splitting some error messages, NL_SET_ERR_MSG_ATTR() can be used
>> instead of GENL_SET_ERR_MSG() in order to give an additional hint to
>> the
>> userspace developers about which attribute is wrong.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/pm_userspace.c | 34 ++++++++++++++++++++++++++--------
>>  1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index
>> 72d4861e497eef5a516fc1c3ec1659807ffedd53..9c0bec588c498e0aa78acf4018d
>> b509abe90cad2 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c

(...)

>> @@ -592,9 +603,16 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
>> *skb, struct genl_info *info)
>>  			goto set_flags_err;
>>  	}
>>  
>> -	if (loc.addr.family == AF_UNSPEC ||
>> -	    rem.addr.family == AF_UNSPEC) {
>> -		GENL_SET_ERR_MSG(info, "invalid address families");
>> +	if (loc.addr.family == AF_UNSPEC) {
>> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
>> +				    "invalid local address family");
>> +		ret = -EINVAL;
>> +		goto set_flags_err;
>> +	}
>> +
>> +	if (rem.addr.family == AF_UNSPEC) {
> 
> I moved this check into "if (attr_rem) {" block in v7.

If you do that, you are changing the behaviour: this attribute is then
no longer mandatory with the userspace PM. I don't think we should do
that, or at least, not as part of a refactoring as you did.

If we want to change the behaviour, we need an explicit patch for that.
See my patch description from patch v6 4/7, and also the
GENL_REQ_ATTR_CHECK() check added in patch v6 7/7.

> 
>> +		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
>> +				    "invalid remote address
>> family");
>>  		ret = -EINVAL;
>>  		goto set_flags_err;
>>  	}
>>
> 

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