netdev - Re: [Draft PATCH net-next 0/3] add YAML spec for team

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231213083642.1872702f@kernel.org>
Date: Wed, 13 Dec 2023 08:36:42 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>, Paolo Abeni
 <pabeni@...hat.com>
Subject: Re: [Draft PATCH net-next 0/3] add YAML spec for team

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...