[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0ae995dd47329e1422cb0e99b7960615c58d37fe.camel@sipsolutions.net>
Date: Fri, 24 Feb 2023 09:33:51 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Jakub Kicinski <kuba@...nel.org>, stephen@...workplumber.org
Cc: dsahern@...il.com, jhs@...atatu.com, netdev@...r.kernel.org
Subject: Re: [PATCH iproute2] genl: print caps for all families
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@...nel.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
Powered by blists - more mailing lists