From: Jakub Kicinski <kuba@kernel•org>
To: Hangbin Liu <liuhangbin@gmail•com>
Cc: netdev@vger•kernel.org, Jiri Pirko <jiri@resnulli•us>,
Paolo Abeni <pabeni@redhat•com>
Subject: Re: [Draft PATCH net-next 0/3] add YAML spec for team
Date: Wed, 13 Dec 2023 08:36:42 -0800 [thread overview]
Message-ID: <20231213083642.1872702f@kernel.org> (raw)
In-Reply-To: <20231213084502.4042718-1-liuhangbin@gmail.com>
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...
next prev parent reply other threads:[~2023-12-13 16:36 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-13 8:44 [Draft PATCH net-next 0/3] add YAML spec for team Hangbin Liu
2023-12-13 8:45 ` [Draft PATCH net-next 1/3] Documentation: netlink: add a " Hangbin Liu
2023-12-13 15:36 ` Jiri Pirko
2023-12-14 3:44 ` Hangbin Liu
2023-12-14 8:25 ` Jiri Pirko
2023-12-13 16:18 ` Jakub Kicinski
2023-12-14 3:52 ` Hangbin Liu
2023-12-14 19:14 ` Jakub Kicinski
2023-12-13 8:45 ` [Draft PATCH net-next 2/3] net: team: use policy generated by YAML spec Hangbin Liu
2023-12-13 15:44 ` Jiri Pirko
2023-12-13 8:45 ` [Draft PATCH net-next 3/3] uapi: team: use header file generated from " Hangbin Liu
2023-12-13 15:39 ` Jiri Pirko
2023-12-13 16:36 ` Jakub Kicinski [this message]
2023-12-14 4:15 ` [Draft PATCH net-next 0/3] add YAML spec for team Hangbin Liu
2023-12-14 19:17 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231213083642.1872702f@kernel.org \
--to=kuba@kernel$(echo .)org \
--cc=jiri@resnulli$(echo .)us \
--cc=liuhangbin@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox