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

[Jamal Hadi Salim <jhs@mojatatu.com>]

After a couple of sips of some unknown drink: I think we can get rid
of ctrl_v altogether as a param to the printers and we should be good
(it would work for events as well).

cheers,
jamal

On Fri, Feb 24, 2023 at 10:15 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Fri, Feb 24, 2023 at 3:33 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> > 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()?
>
> Actually, there is a small gotcha with using a global in the patch -
> events (genl ctrl monitor).
> If it works, it will be expensive to load the controller for every event.
>
> cheers,
> jamal
> > johannes