Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Mon, 31 Jan 2022, Matthieu Baerts wrote:

> Hi Geliang,
>
> On 30/01/2022 05:33, Geliang Tang wrote:
>> Hi Matt,
>>
>> Thanks for your review.
>>
>> I don't know why this commit didn't show in the patchwork. The CI seems to
>> have complained about it.
>
> It seems the issue was on kernel.org side, now fixed: your patch is on
> patchwork and lore.
>
>> On Sat, Jan 29, 2022 at 12:53:20PM +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 29/01/2022 07:26, Geliang Tang wrote:
>>>> NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.
>>>
>>> Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.
>>>
>>> And even if it was always the case for 'ip mptcp', there could be other
>>> tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
>>> case but I'm not sure.
>>
>> I think now is the right time to switch to the new API: nla_parse_nested.
>> Since now no other tool uses our mptcp netlink yet, only 'ip mptcp' and
>> 'pm_nl_ctl'. There's no need to consider about the compatibility.
>
> I would like to only consider Open-Source tools but I don't think we
> can. It is unlikely someone wrote a new tool not using NLA_F_NESTED but
> I don't think we can be 100% sure. This is an uAPI break to change this
> I think, no?
>
>> The old
>> API - nla_parse_nested_deprecated - is reserved for the compatibility of
>> the netlinks that already have some user tools using its interface in the
>> non-NLA_F_NESTED way. We are not the case.
>>
>> Switch to the new API can force the user tools to use the NLA_F_NESTED way
>> from now on. That makes sure no user tool uses the non-NLA_F_NESTED way for
>> our mptcp netlink. Otherwise, one day the old API will be abandoned, and
>> then if there're some user tools use the non-NLA_F_NESTED way, we will have
>> to deal with the compatibility at that time. If we switch to the new API
>> now, we can avoid this potential trouble.
>
> I guess the compatibility will need to be kept of a bit of time. This
> function is used in 250+ places in the kernel. I guess it has the
> "deprecated" word in the name to clearly indicate "you should not use
> that for new features" but not to say "in a few months, we will drop
> this, deal with it now.", no?
>

It seems like the meaning of "deprecated" is the former ("don't add new 
calls to this")... but we did add our use of the function in 2020 after it 
was already deprecated! Given the "do not break userspace" rule for kernel 
changes, I don't expect the deprecated to go away and for now I think 
we should leave it unchanged. When we discuss patches at this week's 
meeting I'll see if others agree.

> It would be easier to use nla_parse_nested() instead of
> nla_parse_nested_deprecated() but I don't think we can.
>

--
Mat Martineau
Intel