Re: [PATCH mptcp-next v3] mptcp: use GENL_REQ_ATTR_CHECK for token

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 20/12/2024 10:50, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, 2024-12-16 at 12:24 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 16/12/2024 09:31, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> A more general way to check if MPTCP_PM_ATTR_TOKEN exists in 'info'
>>> is to use GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_TOKEN) instead of
>>> directly reading info->attrs[MPTCP_PM_ATTR_TOKEN] and then checking
>>> if it's NULL.
>>>
>>> So this patch uses GENL_REQ_ATTR_CHECK() for 'token' in 'info' in
>>> mptcp_userspace_pm_get_sock().
>>>
>>> 'Suggested-by: Jakub Kicinski <kuba@kernel.org>'
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>> v3:
>>>  - use GENL_REQ_ATTR_CHECK in mptcp_userspace_pm_get_sock only
>>>  - drop GENL_SET_ERR_MSG as Matt suggested (thanks)
>>
>> Thank you for the v3. While at it, do you want to do a similar change
>> everywhere we do:
>>
>>   struct nlattr *X = info->attrs[MPTCP_PM_ATTR_Y]:
>>   if (!X) {
>>       GENL_SET_ERR_MSG(info, "missing ...");
>>       return Z;
>>   }
>>
>> e.g. in mptcp_pm_nl_announce_doit() (addr), mptcp_pm_nl_remove_doit()
>> (id), mptcp_pm_nl_subflow_create_doit() (raddr, laddr),
>> mptcp_pm_nl_subflow_destroy_doit() (raddr, laddr).
>> (and maybe mptcp_pm_parse_pm_addr_attr(), but it needs more changes
>> around to pass the attribute ID, probably best not to change that)
> 
> Thanks for this suggestion, I updated this in v4.

Thank you!

>> Also, do you know if 'pm_nl_ctl' will continue to indicate a helpful
>> message if such attribute is missing (that's a message more for
> 
> 'pm_nl_ctl' can't display nl error msg, this is a bug in nl_error():
> 
>  $ sudo ip mptcp endpoint del id 100
>  Error: address not found.
>  $ sudo ./pm_nl_ctl del 100
>  netlink error -22 (Invalid argument)
>  ./pm_nl_ctl: bailing out due to netlink error[s]
> 
> Look at this test, nl error msg "address not found" in
> mptcp_pm_nl_del_addr_doit() can be displayed in 'ip mptcp', but not in
> 'pm_nl_ctl'. I'll try to fix it.

It might help developers if pm_nl_ctl could also print errors generated
by GENL_REQ_ATTR_CHECK(), not only the ones generated with
GENL_SET_ERR_MSG().

>> userspace app dev)? Or does it need to be modified to display the
>> missing argument set by GENL_REQ_ATTR_CHECK()?
> 
> I think no need to modify this, since after GENL_REQ_ATTR_CHECK(), we
> will also call NL_SET_ERR_MSG_ATTR/GENL_SET_ERR_MSG to set nl error msg
> in the error paths.

If GENL_REQ_ATTR_CHECK() is used, it should no longer needed to use
NL_SET_ERR_MSG_ATTR/GENL_SET_ERR_MSG. I guess the userspace should be
able to find the arguments that are missing, no?

This can be done in another series of course.

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