| Message ID | 20231213084502.4042718-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add YAML spec for team | expand |
On Wed, 13 Dec 2023 16:44:59 +0800 Hangbin Liu wrote: > You suggested me to add yaml spec for bridge. Since I'm not familiar with > writing the spec file, I choose to convert team as a start. Nice work! If you write a spec you don't necessarily have to use the spec for C code gen, but I will obviously not stop you from going the extra mile :) > There are still some questions I got during convertion. > > 1. Is there a preference to use "-" instead of "_" for the names in spec file? > e.g. the attr-cnt-name in team.spec, should I use __team-attr-item-port-max > or --team-attr-item-port-max, or __team_attr_item_port_max? Minor preference for using -, but it mostly matters for things which will be visible outside of C. For instance in attr names when they are used in python: msg['port-index'] looks nicer to me than msg['port_index'] and is marginally easier to type. But cnt-name is a C thing, so up to you. If I was writing it myself I'd probably go with --team-attr-item-port-max, that's what MPTCP did. > 2. I saw ynl-gen-c.py deals with unterminated-ok. But this policy is not shown > in the schemas. Is it a new feature that still working on? I must have added it to the code gen when experimenting with a family I didn't end up supporting. I'm not actively working on that one, feel free to take a stab at finishing it or LMK if you need help. > 3. Do we have to hard code the string max-len? Is there a way to use > the name in definitions? e.g. > name: name > type: string > checks: > max-len: string-max-len Yes, that's the intention, if codegen doesn't support that today it should be improved. > 4. The doc will be generate to rst file in future, so there will not have > other comments in the _nl.c or _nl.h files, right? It already generates ReST: https://docs.kernel.org/next/networking/netlink_spec/ We do still generate kdoc in the uAPI header, tho. > 5. the genl_multicast_group is forced to use list. But the team use format > like { .name = TEAM_GENL_CHANGE_EVENT_MC_GRP_NAME, }. Should we support > this legacy format? Do you mean that we generate: [ID] = { "name", } rather than: [ID] = { .name = "name", } ? I think the struct had only one member at the time, so I didn't bother adding the .name, but you can change the code-gen. > 6. The _UAPI_LINUX_IF_TEAM_H_ is rename to _UAPI_LINUX_IF_TEAM_H in uapi > header. Does that affects? Hopefully it's fine. Let's try to make the change and deal with problems if any get reported. Having the standardized guards helps a little bit in our Makefile magic... > 7. When build, I got error modpost: missing MODULE_LICENSE() in drivers/net/team/team_nl.o. > Should we add the MODULE_LICENSE support in ynl-gen-c.py? Not sure if we can, the generated code should be linked with the implementation to form a full module. The manually written part of the implementation should define the license. YAML specs have a fairly odd / broadly open license because they are uAPI. We don't want to start getting into licensing business. > 8. When build, I also got errors like > ERROR: modpost: "team_nl_policy" [drivers/net/team/team.ko] undefined! > ERROR: modpost: "team_nl_ops" [drivers/net/team/team.ko] undefined! > ERROR: modpost: "team_nl_noop_doit" [drivers/net/team/team_nl.ko] undefined! > ERROR: modpost: "team_nl_options_set_doit" [drivers/net/team/team_nl.ko] undefined! > ERROR: modpost: "team_nl_options_get_doit" [drivers/net/team/team_nl.ko] undefined! > ERROR: modpost: "team_nl_port_list_get_doit" [drivers/net/team/team_nl.ko] undefined! > ERROR: modpost: "team_attr_option_nl_policy" [drivers/net/team/team.ko] undefined! > Do you know why include "team_nl.h" doesn't help? Same reason as the reason you're getting the LICENSE warning. kbuild is probably trying to build team_nl and team as separate modules. I think you'll have to rename team.c, take a look at what I did around commit 08d323234d10. I don't know a better way...
Hi Jakub, On Wed, Dec 13, 2023 at 08:36:42AM -0800, Jakub Kicinski wrote: > > There are still some questions I got during convertion. > > > > 1. Is there a preference to use "-" instead of "_" for the names in spec file? > > e.g. the attr-cnt-name in team.spec, should I use __team-attr-item-port-max > > or --team-attr-item-port-max, or __team_attr_item_port_max? > > Minor preference for using -, but it mostly matters for things which > will be visible outside of C. For instance in attr names when they are > used in python: > msg['port-index'] > looks nicer to me than > msg['port_index'] > and is marginally easier to type. But cnt-name is a C thing, so up to > you. If I was writing it myself I'd probably go with > --team-attr-item-port-max, that's what MPTCP did. Thanks, I will do as what else do > > > 2. I saw ynl-gen-c.py deals with unterminated-ok. But this policy is not shown > > in the schemas. Is it a new feature that still working on? > > I must have added it to the code gen when experimenting with a family > I didn't end up supporting. I'm not actively working on that one, feel > free to take a stab at finishing it or LMK if you need help. OK, I will do it. > > > 3. Do we have to hard code the string max-len? Is there a way to use > > the name in definitions? e.g. > > name: name > > type: string > > checks: > > max-len: string-max-len > > Yes, that's the intention, if codegen doesn't support that today it > should be improved. I can try improve this. But may a little late (should go next year). If you have time you can improve this directly. > > > 4. The doc will be generate to rst file in future, so there will not have > > other comments in the _nl.c or _nl.h files, right? > > It already generates ReST: > https://docs.kernel.org/next/networking/netlink_spec/ > We do still generate kdoc in the uAPI header, tho. How to generate the doc in uAPI header? > > > 5. the genl_multicast_group is forced to use list. But the team use format > > like { .name = TEAM_GENL_CHANGE_EVENT_MC_GRP_NAME, }. Should we support > > this legacy format? > > Do you mean that we generate: > > [ID] = { "name", } > > rather than: > > [ID] = { .name = "name", } > > ? I think the struct had only one member at the time, so I didn't > bother adding the .name, but you can change the code-gen. No, the current team just do like static const struct genl_multicast_group team_nl_mcgrps[] = { { .name = TEAM_GENL_CHANGE_EVENT_MC_GRP_NAME, }, }; So there is new ID was defined. Looks the group id is not a must. > > 7. When build, I got error modpost: missing MODULE_LICENSE() in drivers/net/team/team_nl.o. > > Should we add the MODULE_LICENSE support in ynl-gen-c.py? > > Not sure if we can, the generated code should be linked with > the implementation to form a full module. The manually written > part of the implementation should define the license. YAML specs > have a fairly odd / broadly open license because they are uAPI. > We don't want to start getting into licensing business. > > > 8. When build, I also got errors like > > ERROR: modpost: "team_nl_policy" [drivers/net/team/team.ko] undefined! > > ERROR: modpost: "team_nl_ops" [drivers/net/team/team.ko] undefined! > > ERROR: modpost: "team_nl_noop_doit" [drivers/net/team/team_nl.ko] undefined! > > ERROR: modpost: "team_nl_options_set_doit" [drivers/net/team/team_nl.ko] undefined! > > ERROR: modpost: "team_nl_options_get_doit" [drivers/net/team/team_nl.ko] undefined! > > ERROR: modpost: "team_nl_port_list_get_doit" [drivers/net/team/team_nl.ko] undefined! > > ERROR: modpost: "team_attr_option_nl_policy" [drivers/net/team/team.ko] undefined! > > Do you know why include "team_nl.h" doesn't help? > > Same reason as the reason you're getting the LICENSE warning. > kbuild is probably trying to build team_nl and team as separate modules. > > I think you'll have to rename team.c, take a look at what I did around > commit 08d323234d10. I don't know a better way... Thanks, this works for me. Cheers Hangbin
On Thu, 14 Dec 2023 12:15:07 +0800 Hangbin Liu wrote: > > > 3. Do we have to hard code the string max-len? Is there a way to use > > > the name in definitions? e.g. > > > name: name > > > type: string > > > checks: > > > max-len: string-max-len > > > > Yes, that's the intention, if codegen doesn't support that today it > > should be improved. > > I can try improve this. But may a little late (should go next year). > If you have time you can improve this directly. Noted on my todo list but no ETA, let's see who gets to it first.. :) > > > 4. The doc will be generate to rst file in future, so there will not have > > > other comments in the _nl.c or _nl.h files, right? > > > > It already generates ReST: > > https://docs.kernel.org/next/networking/netlink_spec/ > > We do still generate kdoc in the uAPI header, tho. > > How to generate the doc in uAPI header? The doc strings for enum types should appear in uAPI. Other docs in uAPI usually describe nesting.. which the YAML spec makes a bit obsolete / possible to generate automatically. If there's more that we need we can extend the codegen..
Hi Jakub, You suggested me to add yaml spec for bridge. Since I'm not familiar with writing the spec file, I choose to convert team as a start. There are still some questions I got during convertion. 1. Is there a preference to use "-" instead of "_" for the names in spec file? e.g. the attr-cnt-name in team.spec, should I use __team-attr-item-port-max or --team-attr-item-port-max, or __team_attr_item_port_max? 2. I saw ynl-gen-c.py deals with unterminated-ok. But this policy is not shown in the schemas. Is it a new feature that still working on? 3. Do we have to hard code the string max-len? Is there a way to use the name in definitions? e.g. name: name type: string checks: max-len: string-max-len 4. The doc will be generate to rst file in future, so there will not have other comments in the _nl.c or _nl.h files, right? 5. the genl_multicast_group is forced to use list. But the team use format like { .name = TEAM_GENL_CHANGE_EVENT_MC_GRP_NAME, }. Should we support this legacy format? 6. The _UAPI_LINUX_IF_TEAM_H_ is rename to _UAPI_LINUX_IF_TEAM_H in uapi header. Does that affects? 7. When build, I got error modpost: missing MODULE_LICENSE() in drivers/net/team/team_nl.o. Should we add the MODULE_LICENSE support in ynl-gen-c.py? 8. When build, I also got errors like ERROR: modpost: "team_nl_policy" [drivers/net/team/team.ko] undefined! ERROR: modpost: "team_nl_ops" [drivers/net/team/team.ko] undefined! ERROR: modpost: "team_nl_noop_doit" [drivers/net/team/team_nl.ko] undefined! ERROR: modpost: "team_nl_options_set_doit" [drivers/net/team/team_nl.ko] undefined! ERROR: modpost: "team_nl_options_get_doit" [drivers/net/team/team_nl.ko] undefined! ERROR: modpost: "team_nl_port_list_get_doit" [drivers/net/team/team_nl.ko] undefined! ERROR: modpost: "team_attr_option_nl_policy" [drivers/net/team/team.ko] undefined! Do you know why include "team_nl.h" doesn't help? Thanks Hangbin Hangbin Liu (3): Documentation: netlink: add a YAML spec for team net: team: use policy generated by YAML spec uapi: team: use header file generated from YAML spec Documentation/netlink/specs/team.yaml | 205 ++++++++++++++++++++++++++ MAINTAINERS | 1 + drivers/net/team/Makefile | 2 +- drivers/net/team/team.c | 59 +------- drivers/net/team/team_nl.c | 59 ++++++++ drivers/net/team/team_nl.h | 29 ++++ include/uapi/linux/if_team.h | 116 ++++++--------- 7 files changed, 345 insertions(+), 126 deletions(-) create mode 100644 Documentation/netlink/specs/team.yaml create mode 100644 drivers/net/team/team_nl.c create mode 100644 drivers/net/team/team_nl.h