[v3] mptcp: convert netlink code to use YAML spec

[PATCH mptcp-next v3 0/6] mptcp: convert netlink code to use YAML spec

Posted by Davide Caratti 7 months, 4 weeks ago

this series converts most of the MPTCP netlink interface (plus uAPI bits)
to use sources generated by a YAML spec file. Patch 2/6 and 6/6 have been
individually verified with kselftests.

POC:

 $ sudo  ./tools/net/ynl/cli.py  --spec \
 > Documentation/netlink/specs/mptcp.yaml --do add_addr \
 > --json '{"addr": {"addr4": 16909061, "family": 2, "flags": 4, "id": 10, "port": 0}}'

 $ ip -j mptcp endpoint show id 10
 [{"address":"1.2.3.5","id":10,"backup":true}]

v3:
- add missing 'static' keyword (MPTCP CI)
- fix element ordering for 'attr' attributes in patch 2,
  mptcp spec and generated C code (Paolo Abeni)
- removed extra newline, deuglified subjects in patch 2 and 4

v2:
- mptcp.yaml: only put values around enum "holes" (Paolo Abeni)
-  _doit and _dumpit renames are done in a dedicate patch (Paolo Abeni)
- removed useless nla_policy passed through parse_entry()  (Paolo Abeni)
- renamed mptcp_pm_address_nl_policy in patch 2 (Paolo Abeni)
- (hopefully) more comprehensible commit messages (Paolo Abeni)

Davide Caratti (6):
  tools: ynl: add uns-admin-perm to genetlink legacy
  net: mptcp: convert netlink from small_ops to ops
  Documentation: netlink: add a YAML spec for mptcp
  uapi: mptcp: use header file generated from YAML spec
  net: mptcp: rename netlink handlers to
    mptcp_pm_nl_<blah>_{doit,dumpit}
  net: mptcp: use policy generated by YAML spec

 Documentation/netlink/genetlink-legacy.yaml |   2 +-
 Documentation/netlink/specs/mptcp.yaml      | 394 ++++++++++++++++++++
 include/uapi/linux/mptcp.h                  | 174 +--------
 include/uapi/linux/mptcp_pm.h               | 149 ++++++++
 net/mptcp/Makefile                          |   3 +-
 net/mptcp/mptcp_pm_gen.c                    | 179 +++++++++
 net/mptcp/mptcp_pm_gen.h                    |  58 +++
 net/mptcp/pm_netlink.c                      | 114 +-----
 net/mptcp/pm_userspace.c                    |   8 +-
 net/mptcp/protocol.h                        |   6 +-
 10 files changed, 814 insertions(+), 273 deletions(-)
 create mode 100644 Documentation/netlink/specs/mptcp.yaml
 create mode 100644 include/uapi/linux/mptcp_pm.h
 create mode 100644 net/mptcp/mptcp_pm_gen.c
 create mode 100644 net/mptcp/mptcp_pm_gen.h

-- 
2.40.1

Re: [PATCH mptcp-next v3 0/6] mptcp: convert netlink code to use YAML spec

Posted by Paolo Abeni 7 months, 3 weeks ago

On Tue, 2023-09-05 at 15:53 +0200, Davide Caratti wrote:
> this series converts most of the MPTCP netlink interface (plus uAPI bits)
> to use sources generated by a YAML spec file. Patch 2/6 and 6/6 have been
> individually verified with kselftests.
> 
> POC:
> 
>  $ sudo  ./tools/net/ynl/cli.py  --spec \
>  > Documentation/netlink/specs/mptcp.yaml --do add_addr \
>  > --json '{"addr": {"addr4": 16909061, "family": 2, "flags": 4, "id": 10, "port": 0}}'
>  -j mptcp endpoint show id 10
>  [{"address":"1.2.3.5","id":10,"backup":true}]
> 
> v3:
> - add missing 'static' keyword (MPTCP CI)
> - fix element ordering for 'attr' attributes in patch 2,
>   mptcp spec and generated C code (Paolo Abeni)
> - removed extra newline, deuglified subjects in patch 2 and 4
> 
> v2:
> - mptcp.yaml: only put values around enum "holes" (Paolo Abeni)
> -  _doit and _dumpit renames are done in a dedicate patch (Paolo Abeni)
> - removed useless nla_policy passed through parse_entry()  (Paolo Abeni)
> - renamed mptcp_pm_address_nl_policy in patch 2 (Paolo Abeni)
> - (hopefully) more comprehensible commit messages (Paolo Abeni)
> 
> Davide Caratti (6):
>   tools: ynl: add uns-admin-perm to genetlink legacy
>   net: mptcp: convert netlink from small_ops to ops
>   Documentation: netlink: add a YAML spec for mptcp
>   uapi: mptcp: use header file generated from YAML spec
>   net: mptcp: rename netlink handlers to
>     mptcp_pm_nl_<blah>_{doit,dumpit}
>   net: mptcp: use policy generated by YAML spec
> 
>  Documentation/netlink/genetlink-legacy.yaml |   2 +-
>  Documentation/netlink/specs/mptcp.yaml      | 394 ++++++++++++++++++++
>  include/uapi/linux/mptcp.h                  | 174 +--------
>  include/uapi/linux/mptcp_pm.h               | 149 ++++++++
>  net/mptcp/Makefile                          |   3 +-
>  net/mptcp/mptcp_pm_gen.c                    | 179 +++++++++
>  net/mptcp/mptcp_pm_gen.h                    |  58 +++
>  net/mptcp/pm_netlink.c                      | 114 +-----
>  net/mptcp/pm_userspace.c                    |   8 +-
>  net/mptcp/protocol.h                        |   6 +-
>  10 files changed, 814 insertions(+), 273 deletions(-)
>  create mode 100644 Documentation/netlink/specs/mptcp.yaml
>  create mode 100644 include/uapi/linux/mptcp_pm.h
>  create mode 100644 net/mptcp/mptcp_pm_gen.c
>  create mode 100644 net/mptcp/mptcp_pm_gen.h

FWIW, LGTM, TY!

/P

p.s. ;)

Re: [PATCH mptcp-next v3 0/6] mptcp: convert netlink code to use YAML spec

Posted by Matthieu Baerts 7 months, 2 weeks ago

Hi Davide,

On 05/09/2023 15:53, Davide Caratti wrote:
> this series converts most of the MPTCP netlink interface (plus uAPI bits)
> to use sources generated by a YAML spec file. Patch 2/6 and 6/6 have been
> individually verified with kselftests.

Thank you for the patches!

I was looking at applying them but I just noticed quite a few errors and
warnings reported by CheckPatch. Do you mind looking at them please?

> Davide Caratti (6):
>   tools: ynl: add uns-admin-perm to genetlink legacy>   net: mptcp: convert netlink from small_ops to ops

Spaces are used instead of tabs for the indentation in
include/uapi/linux/mptcp.h and net/mptcp/pm_netlink.c. This code is
removed later but I think it is still best to avoid having many errors
reported by the CI when sending them upstream.

>   Documentation: netlink: add a YAML spec for mptcp

Can you add the path of the new file in the MAINTAINERS file please?
Also good to add a small description in the commit message, at least to
say that it is supposed to represent the YAML spec as currently
available today ("no behaviour change intended").

>   uapi: mptcp: use header file generated from YAML spec

Please add a "*" in the MAINTAINERS file to handle the new one in uapi.

Also, I guess we cannot fix:

  Please use a blank line after function/struct/union/enum declarations

in include/uapi/linux/mptcp_pm.h as it is auto-generated. (Maybe we
should add a comment about that in the commit message? But maybe it is
clear it is auto-generated?)

>   net: mptcp: rename netlink handlers to mptcp_pm_nl_<blah>_{doit,dumpit}
>   net: mptcp: use policy generated by YAML spec

If it is OK to send a v4, please also add Paolo's ACK. I think that what
he meant to say but he was apparently only allowed to send acronyms :-D

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v3 0/6] mptcp: convert netlink code to use YAML spec

Posted by Davide Caratti 7 months, 2 weeks ago

hello Matthieu

On Sat, Sep 16, 2023 at 1:14 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Davide,
>
[...]
> I was looking at applying them but I just noticed quite a few errors and
> warnings reported by CheckPatch. Do you mind looking at them please?
>
> > Davide Caratti (6):
> >   tools: ynl: add uns-admin-perm to genetlink legacy>   net: mptcp: convert netlink from small_ops to ops
>
> Spaces are used instead of tabs for the indentation in
> include/uapi/linux/mptcp.h and net/mptcp/pm_netlink.c. This code is
> removed later but I think it is still best to avoid having many errors
> reported by the CI when sending them upstream.

yes, and it was unintentional. Will fix that today and resend a v4.

>
> >   Documentation: netlink: add a YAML spec for mptcp
>
> Can you add the path of the new file in the MAINTAINERS file please?
> Also good to add a small description in the commit message, at least to
> say that it is supposed to represent the YAML spec as currently
> available today ("no behaviour change intended").

makes sense,

>
> >   uapi: mptcp: use header file generated from YAML spec
>
> Please add a "*" in the MAINTAINERS file to handle the new one in uapi.
>
> Also, I guess we cannot fix:
>
>   Please use a blank line after function/struct/union/enum declarations
>
> in include/uapi/linux/mptcp_pm.h as it is auto-generated. (Maybe we
> should add a comment about that in the commit message? But maybe it is
> clear it is auto-generated?)

this can probably be fixed in a follow-up commit for ynl-gen-c tool
(that includes a tree-wide fix for the uAPI headers, because the issue
might also affect all headers generated until today). The commit
message says how we use the tool to generate sources, and the header
file has the "do not edit" caveat line - that is sufficient IMO.

> >   net: mptcp: rename netlink handlers to mptcp_pm_nl_<blah>_{doit,dumpit}
> >   net: mptcp: use policy generated by YAML spec
>
> If it is OK to send a v4, please also add Paolo's ACK. I think that what
> he meant to say but he was apparently only allowed to send acronyms :-D

OK is an acronym as well AFAIK :)

thanks for looking at this!
-- 
davide

Re: [PATCH mptcp-next v3 0/6] mptcp: convert netlink code to use YAML spec

Posted by Matthieu Baerts 7 months, 2 weeks ago

Hi Davide,

On 18/09/2023 10:07, Davide Caratti wrote:
> hello Matthieu
> 
> On Sat, Sep 16, 2023 at 1:14 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Davide,
>>
> [...]
>> I was looking at applying them but I just noticed quite a few errors and
>> warnings reported by CheckPatch. Do you mind looking at them please?
>>
>>> Davide Caratti (6):
>>>   tools: ynl: add uns-admin-perm to genetlink legacy>   net: mptcp: convert netlink from small_ops to ops
>>
>> Spaces are used instead of tabs for the indentation in
>> include/uapi/linux/mptcp.h and net/mptcp/pm_netlink.c. This code is
>> removed later but I think it is still best to avoid having many errors
>> reported by the CI when sending them upstream.
> 
> yes, and it was unintentional. Will fix that today and resend a v4.

Thanks!

>>>   Documentation: netlink: add a YAML spec for mptcp
>>
>> Can you add the path of the new file in the MAINTAINERS file please?
>> Also good to add a small description in the commit message, at least to
>> say that it is supposed to represent the YAML spec as currently
>> available today ("no behaviour change intended").
> 
> makes sense,

Thanks!

>>>   uapi: mptcp: use header file generated from YAML spec
>>
>> Please add a "*" in the MAINTAINERS file to handle the new one in uapi.
>>
>> Also, I guess we cannot fix:
>>
>>   Please use a blank line after function/struct/union/enum declarations
>>
>> in include/uapi/linux/mptcp_pm.h as it is auto-generated. (Maybe we
>> should add a comment about that in the commit message? But maybe it is
>> clear it is auto-generated?)
> 
> this can probably be fixed in a follow-up commit for ynl-gen-c tool
> (that includes a tree-wide fix for the uAPI headers, because the issue
> might also affect all headers generated until today). The commit
> message says how we use the tool to generate sources, and the header
> file has the "do not edit" caveat line - that is sufficient IMO.

I understand, thank you for the explanation. This improvement can be
done later if needed.

Fine with the current commit message, only the MAINTAINERS file needs to
be modified in this commit then.

>>>   net: mptcp: rename netlink handlers to mptcp_pm_nl_<blah>_{doit,dumpit}
>>>   net: mptcp: use policy generated by YAML spec
>>
>> If it is OK to send a v4, please also add Paolo's ACK. I think that what
>> he meant to say but he was apparently only allowed to send acronyms :-D
> 
> OK is an acronym as well AFAIK :)

:-)

> thanks for looking at this!

You are welcome!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net