| Message ID | cover.1693921470.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mptcp: convert netlink code to use YAML spec | expand |
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. ;)
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
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!
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