Re: [PATCH iproute2] genl: print caps for all families

[Johannes Berg <johannes@sipsolutions.net>]

On Thu, 2023-02-23 at 17:57 -0800, Jakub Kicinski wrote:
> On Thu, 23 Feb 2023 17:52:34 -0800 Jakub Kicinski wrote:
> > Back in 2006 kernel commit 334c29a64507 ("[GENETLINK]: Move
> > command capabilities to flags.") removed some attributes and
> > moved the capabilities to flags. Corresponding iproute2
> > commit 26328fc3933f ("Add controller support for new features
> > exposed") added the ability to print those caps.
> > 
> > Printing is gated on version of the family, but we're checking
> > the version of each individual family rather than the control
> > family. The format of attributes in the control family
> > is dictated by the version of the control family alone.
> > 
> > Families can't use flags for random things, anyway,
> > because kernel core has a fixed interpretation.
> > 
> > Thanks to this change caps will be shown for all families
> > (assuming kernel newer than 2.6.19), not just those which
> > by coincidence have their local version >= 2.
> > 
> > For instance devlink, before:
> > 
> >   $ genl ctrl get name devlink
> >   Name: devlink
> > 	ID: 0x15  Version: 0x1  header size: 0  max attribs: 179
> > 	commands supported:
> > 		#1:  ID-0x1
> > 		#2:  ID-0x5
> > 		#3:  ID-0x6
> > 		...
> > 
> > after:
> > 
> >   $ genl ctrl get name devlink
> >   Name: devlink
> > 	ID: 0x15  Version: 0x1  header size: 0  max attribs: 179
> > 	commands supported:
> > 		#1:  ID-0x1
> > 		Capabilities (0xe):
> >  		  can doit; can dumpit; has policy
> > 
> > 		#2:  ID-0x5
> > 		Capabilities (0xe):
> >  		  can doit; can dumpit; has policy
> > 
> > 		#3:  ID-0x6
> > 		Capabilities (0xb):
> >  		  requires admin permission; can doit; has policy
> > 
> > Leave ctrl_v as 0 if we fail to read the version. Old code used 1
> > as the default, but 0 or 1 - does not matter, checks are for >= 2.
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > Not really sure if this is a fix or not..
> 
> Adding Johannes, that's probably everyone who ever used this 
> command on CC? ;)

Hehe. I'm not even sure I use(d) that part of it frequently ;-)

> > --- a/genl/ctrl.c
> > +++ b/genl/ctrl.c
> > @@ -21,6 +21,8 @@
> >  #define GENL_MAX_FAM_OPS	256
> >  #define GENL_MAX_FAM_GRPS	256
> >  
> > +static unsigned int ctrl_v;

You know I looked at this on my phone this morning and missed the fact
that it's iproute2, and was wondering what you're doing with a global
variable in the kernel ;-)

There's this code also:

> static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
> ...
> static int print_ctrl_grp(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)

and it feels a bit pointless to pass a now global ctrl_v to the function
arguments?

> > @@ -264,6 +313,9 @@ static int ctrl_list(int cmd, int argc, char **argv)
> >  		exit(1);
> >  	}
> >  
> > +	if (!ctrl_v)
> > +		ctrl_load_ctrl_version(&rth);

You call this here, but what about this:

> struct genl_util ctrl_genl_util = {
>         .name = "ctrl",
>         .parse_genlopt = parse_ctrl,
>         .print_genlopt = print_ctrl2,
> };

where print_ctrl2 and hence all the above will be called with a now zero
ctrl_v, whereas before it would've been - at least in some cases? -
initialized by ctrl_list() itself?


Oh. I see now. The issue was which version we use - the family version
vs. the controller version. How did I miss that until here ...

Still it seems it should be always initialized in print_ctrl rather than
in ctrl_list, to capture the case of print_ctrl2? Or maybe in there, but
that's called inside ctrl_list(), so maybe have parse_ctrl() already
initialize it, rather than ctrl_list()?

johannes