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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 14 Dec 2023 12:15:07 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
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

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