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

[Geliang Tang <geliang.tang@suse.com>]

On Fri, Feb 04, 2022 at 11:16:21AM +0100, Paolo Abeni wrote:
> On Wed, 2022-02-02 at 16:38 -0800, Mat Martineau wrote:
> > 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.
> 
> All known users (self-tests and iproute2) have NLA_F_NESTED since the
> first implementation and I doubt there are other user-space users out
> there.
>  
> Still the downside of breaking user-space are quite bad, and there are
> other in kernel users of the deprecated api added even later then
> MPTCP.
> 
> I think we can keep the existing code.

Agree.

Thanks,
-Geliang

> 
> Cheers,
> 
> Paolo
>