[PATCH iproute2] genl: print caps for all families

[PATCH iproute2] genl: print caps for all families

[Jakub Kicinski]

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..
---
 genl/ctrl.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/genl/ctrl.c b/genl/ctrl.c
index a2d87af0ad07..1fcb3848f137 100644
--- 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;
+
 static int usage(void)
 {
 	fprintf(stderr,"Usage: ctrl <CMD>\n" \
@@ -109,7 +111,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
 	int len = n->nlmsg_len;
 	struct rtattr *attrs;
 	FILE *fp = (FILE *) arg;
-	__u32 ctrl_v = 0x1;
 
 	if (n->nlmsg_type !=  GENL_ID_CTRL) {
 		fprintf(stderr, "Not a controller message, nlmsg_len=%d "
@@ -148,7 +149,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
 	if (tb[CTRL_ATTR_VERSION]) {
 		__u32 *v = RTA_DATA(tb[CTRL_ATTR_VERSION]);
 		fprintf(fp, " Version: 0x%x ",*v);
-		ctrl_v = *v;
 	}
 	if (tb[CTRL_ATTR_HDRSIZE]) {
 		__u32 *h = RTA_DATA(tb[CTRL_ATTR_HDRSIZE]);
@@ -241,6 +241,55 @@ static int print_ctrl2(struct nlmsghdr *n, void *arg)
 	return print_ctrl(NULL, n, arg);
 }
 
+static void ctrl_load_ctrl_version(struct rtnl_handle *rth)
+{
+	struct rtattr *tb[CTRL_ATTR_MAX + 1];
+	struct genlmsghdr *ghdr;
+	struct rtattr *attrs;
+	struct {
+		struct nlmsghdr         n;
+		struct genlmsghdr	g;
+		char                    buf[128];
+	} req = {
+		.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN),
+		.n.nlmsg_flags = NLM_F_REQUEST,
+		.n.nlmsg_type = GENL_ID_CTRL,
+		.g.cmd = CTRL_CMD_GETFAMILY,
+	};
+	struct nlmsghdr *nlh = &req.n;
+	struct nlmsghdr *answer = NULL;
+	__u16 id = GENL_ID_CTRL;
+	int len;
+
+	addattr_l(nlh, 128, CTRL_ATTR_FAMILY_ID, &id, 2);
+
+	if (rtnl_talk(rth, nlh, &answer) < 0) {
+		fprintf(stderr, "Error talking to the kernel\n");
+		return;
+	}
+
+	ghdr = NLMSG_DATA(answer);
+	if (answer->nlmsg_type != GENL_ID_CTRL ||
+	    ghdr->cmd != CTRL_CMD_NEWFAMILY) {
+		fprintf(stderr, "Unexpected reply nlmsg_type=%u cmd=%u\n",
+			answer->nlmsg_type, ghdr->cmd);
+		return;
+	}
+
+	len = answer->nlmsg_len;
+	len -= NLMSG_LENGTH(GENL_HDRLEN);
+	if (len < 0) {
+		fprintf(stderr, "wrong controller message len %d\n", len);
+		return;
+	}
+
+	attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN);
+	parse_rtattr_flags(tb, CTRL_ATTR_MAX, attrs, len, NLA_F_NESTED);
+
+	if (tb[CTRL_ATTR_VERSION])
+		ctrl_v = rta_getattr_u16(tb[CTRL_ATTR_VERSION]);
+}
+
 static int ctrl_list(int cmd, int argc, char **argv)
 {
 	struct rtnl_handle rth;
@@ -264,6 +313,9 @@ static int ctrl_list(int cmd, int argc, char **argv)
 		exit(1);
 	}
 
+	if (!ctrl_v)
+		ctrl_load_ctrl_version(&rth);
+
 	if (cmd == CTRL_CMD_GETFAMILY || cmd == CTRL_CMD_GETPOLICY) {
 		req.g.cmd = cmd;
 
-- 
2.39.2

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

[Jakub Kicinski]

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? ;)

> diff --git a/genl/ctrl.c b/genl/ctrl.c
> index a2d87af0ad07..1fcb3848f137 100644
> --- 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;
> +
>  static int usage(void)
>  {
>  	fprintf(stderr,"Usage: ctrl <CMD>\n" \
> @@ -109,7 +111,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>  	int len = n->nlmsg_len;
>  	struct rtattr *attrs;
>  	FILE *fp = (FILE *) arg;
> -	__u32 ctrl_v = 0x1;
>  
>  	if (n->nlmsg_type !=  GENL_ID_CTRL) {
>  		fprintf(stderr, "Not a controller message, nlmsg_len=%d "
> @@ -148,7 +149,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>  	if (tb[CTRL_ATTR_VERSION]) {
>  		__u32 *v = RTA_DATA(tb[CTRL_ATTR_VERSION]);
>  		fprintf(fp, " Version: 0x%x ",*v);
> -		ctrl_v = *v;
>  	}
>  	if (tb[CTRL_ATTR_HDRSIZE]) {
>  		__u32 *h = RTA_DATA(tb[CTRL_ATTR_HDRSIZE]);
> @@ -241,6 +241,55 @@ static int print_ctrl2(struct nlmsghdr *n, void *arg)
>  	return print_ctrl(NULL, n, arg);
>  }
>  
> +static void ctrl_load_ctrl_version(struct rtnl_handle *rth)
> +{
> +	struct rtattr *tb[CTRL_ATTR_MAX + 1];
> +	struct genlmsghdr *ghdr;
> +	struct rtattr *attrs;
> +	struct {
> +		struct nlmsghdr         n;
> +		struct genlmsghdr	g;
> +		char                    buf[128];
> +	} req = {
> +		.n.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN),
> +		.n.nlmsg_flags = NLM_F_REQUEST,
> +		.n.nlmsg_type = GENL_ID_CTRL,
> +		.g.cmd = CTRL_CMD_GETFAMILY,
> +	};
> +	struct nlmsghdr *nlh = &req.n;
> +	struct nlmsghdr *answer = NULL;
> +	__u16 id = GENL_ID_CTRL;
> +	int len;
> +
> +	addattr_l(nlh, 128, CTRL_ATTR_FAMILY_ID, &id, 2);
> +
> +	if (rtnl_talk(rth, nlh, &answer) < 0) {
> +		fprintf(stderr, "Error talking to the kernel\n");
> +		return;
> +	}
> +
> +	ghdr = NLMSG_DATA(answer);
> +	if (answer->nlmsg_type != GENL_ID_CTRL ||
> +	    ghdr->cmd != CTRL_CMD_NEWFAMILY) {
> +		fprintf(stderr, "Unexpected reply nlmsg_type=%u cmd=%u\n",
> +			answer->nlmsg_type, ghdr->cmd);
> +		return;
> +	}
> +
> +	len = answer->nlmsg_len;
> +	len -= NLMSG_LENGTH(GENL_HDRLEN);
> +	if (len < 0) {
> +		fprintf(stderr, "wrong controller message len %d\n", len);
> +		return;
> +	}
> +
> +	attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN);
> +	parse_rtattr_flags(tb, CTRL_ATTR_MAX, attrs, len, NLA_F_NESTED);
> +
> +	if (tb[CTRL_ATTR_VERSION])
> +		ctrl_v = rta_getattr_u16(tb[CTRL_ATTR_VERSION]);
> +}
> +
>  static int ctrl_list(int cmd, int argc, char **argv)
>  {
>  	struct rtnl_handle rth;
> @@ -264,6 +313,9 @@ static int ctrl_list(int cmd, int argc, char **argv)
>  		exit(1);
>  	}
>  
> +	if (!ctrl_v)
> +		ctrl_load_ctrl_version(&rth);
> +
>  	if (cmd == CTRL_CMD_GETFAMILY || cmd == CTRL_CMD_GETPOLICY) {
>  		req.g.cmd = cmd;
>  

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

[Johannes Berg]

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

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

[Jamal Hadi Salim]

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

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

[Jamal Hadi Salim]

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

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

[Jakub Kicinski]

On Fri, 24 Feb 2023 10:22:56 -0500 Jamal Hadi Salim wrote:
> 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).

Do you mean to always interpret the flags? FWIW I think that should 
be fine, old kernels just don't set those flags so I'm not sure why
we're gating interpreting them with the version.

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

[Jamal Hadi Salim]

On Fri, Feb 24, 2023 at 12:10 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 24 Feb 2023 10:22:56 -0500 Jamal Hadi Salim wrote:
> > 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).
>
> Do you mean to always interpret the flags? FWIW I think that should
> be fine, old kernels just don't set those flags so I'm not sure why
> we're gating interpreting them with the version.

I think it was leftover cruft from when the capabilities were being
sent as attributes. Old kernels
should work fine AFAIK.

cheers,
jamal

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

[Jakub Kicinski]

On Fri, 24 Feb 2023 12:46:26 -0500 Jamal Hadi Salim wrote:
> > Do you mean to always interpret the flags? FWIW I think that should
> > be fine, old kernels just don't set those flags so I'm not sure why
> > we're gating interpreting them with the version.  
> 
> I think it was leftover cruft from when the capabilities were being
> sent as attributes. Old kernels
> should work fine AFAIK.

Thanks, SG! 
I'll send v2 which removes the version checks completely later.

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

[Stephen Hemminger]

On Thu, 23 Feb 2023 17:52:34 -0800
Jakub Kicinski <kuba@kernel.org> 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>

What about JSON support. Is genl not json ready yet?

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

[Jakub Kicinski]

On Thu, 23 Feb 2023 19:27:42 -0800 Stephen Hemminger wrote:
> What about JSON support. Is genl not json ready yet?

All the genl code looks quite dated, no JSON anywhere in sight :(

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

[Jamal Hadi Salim]

On Fri, Feb 24, 2023 at 12:11 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 23 Feb 2023 19:27:42 -0800 Stephen Hemminger wrote:
> > What about JSON support. Is genl not json ready yet?
>
> All the genl code looks quite dated, no JSON anywhere in sight :(

We'll take care of this...

cheers,
jamal

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

[Jakub Kicinski]

On Fri, 24 Feb 2023 12:47:02 -0500 Jamal Hadi Salim wrote:
> On Fri, Feb 24, 2023 at 12:11 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 23 Feb 2023 19:27:42 -0800 Stephen Hemminger wrote:  
> > > What about JSON support. Is genl not json ready yet?  
> >
> > All the genl code looks quite dated, no JSON anywhere in sight :(  
> 
> We'll take care of this...

I'm biased but at this point the time is probably better spent trying
to filling the gaps in ynl than add JSON to a CLI tool nobody knows
about... too harsh? :)

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

[Stephen Hemminger]

On Fri, 24 Feb 2023 10:29:35 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 24 Feb 2023 12:47:02 -0500 Jamal Hadi Salim wrote:
> > On Fri, Feb 24, 2023 at 12:11 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > > On Thu, 23 Feb 2023 19:27:42 -0800 Stephen Hemminger wrote:    
> > > > What about JSON support. Is genl not json ready yet?    
> > >
> > > All the genl code looks quite dated, no JSON anywhere in sight :(    
> > 
> > We'll take care of this...  
> 
> I'm biased but at this point the time is probably better spent trying
> to filling the gaps in ynl than add JSON to a CLI tool nobody knows
> about... too harsh? :

So I can drop it (insert sarcasm here)

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

[Jakub Kicinski]

On Fri, 24 Feb 2023 14:33:27 -0800 Stephen Hemminger wrote:
> > I'm biased but at this point the time is probably better spent trying
> > to filling the gaps in ynl than add JSON to a CLI tool nobody knows
> > about... too harsh? :  
> 
> So I can drop it (insert sarcasm here)

It may be useful to experts as a for-human-consumption CLI.

JSON to me implies use in scripts or higher level code, I can't
think of a reason why scripts would poke into genl internals.
E.g. if a script wants something from devlink it will call devlink,
and the devlink tool internally may interrogate genl internals.

IOW we're tapping into a layer in the middle of the tech stack, 
while user wants JSON out of the end of the stack.

[We can replace ynl with a iproute2 internal lib or libml to avoid bias]
Now, what I'm saying is - for devlink - we don't have a easy to use
library which a programmer can interact with to query the family info.
Parsing thru the policy dumps is _hard_. So building a C library for
this seems more fruitful than adding JSON to the CLI tool.

IDK if I'm making sense.  I could well be wrong.

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

[Jamal Hadi Salim]

On Fri, Feb 24, 2023 at 7:56 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 24 Feb 2023 14:33:27 -0800 Stephen Hemminger wrote:
> > > I'm biased but at this point the time is probably better spent trying
> > > to filling the gaps in ynl than add JSON to a CLI tool nobody knows
> > > about... too harsh? :
> >
> > So I can drop it (insert sarcasm here)
>
> It may be useful to experts as a for-human-consumption CLI.
>
> JSON to me implies use in scripts or higher level code, I can't
> think of a reason why scripts would poke into genl internals.
> E.g. if a script wants something from devlink it will call devlink,
> and the devlink tool internally may interrogate genl internals.
>

genl was/is mostly useful for debugging. Ive used it on/off to check
on things but on its own not much use.

> IOW we're tapping into a layer in the middle of the tech stack,
> while user wants JSON out of the end of the stack.
>

Only reason to make it json capable is for consistency with the rest
of iproute2 (given the effort to convert all prints to json). Main
value for json is providing a more formal, parseable output. Probably
not for genl, but generally for iproute2 utilities, example tc,
running tc -j | jq somethinghere | grep ..  is more predictable in
outcome than using text output piped to grep (which is what people
used to do before  -j); we extensively use it in tdc for example.

> [We can replace ynl with a iproute2 internal lib or libml to avoid bias]
> Now, what I'm saying is - for devlink - we don't have a easy to use
> library which a programmer can interact with to query the family info.
> Parsing thru the policy dumps is _hard_. So building a C library for
> this seems more fruitful than adding JSON to the CLI tool.
>
> IDK if I'm making sense.  I could well be wrong.

Not familiar with ynl, but if it is a library then I would suggest it
be part of iproute2 to avoid code replication etc. The LinuxWay(tm) is
CutnpasteThenEditAway - a lot of code in iproute2 is very templateable
so would benefit that way. Such code is also susceptible to human
error (see Pedros patches to fix broken usage of "index" parsing in tc
actions).

Re Devlink (or any other users): one of the original goals of generic
netlink was to seek the source of truth from the kernel via
introspection to discover properties of the different users, their
families, commands, etc and then formulate what will be acceptable to
send from user space. Some of that code unfortunately never went in
(mostly because it was an idea that I had but no users existed at the
time); Johannes I believe introduced policy discovery at some point
which may make it feasible now. The idea then is the CLI would query
the kernel for the properties, build enough details and then process
user requirements with the known constraints. Maybe that idea is still
too ambitious.

For sure having a centralized CLI that is widely adopted helps - a lot
of the code is very templa. It's a lot easier to evolve current tools
than to replace them even when the evolution is superficial. Folks
familiar with "ip" or "tc" (and already have the certification to
prove it) would be happier to stick with those interfaces than
creating a new one. If you are a developer and want to write your own
application then having well formed libraries (probably polyglot) is
very valuable; however, that doesnt contradict the need for
operational tooling such as provided by iproute2.

I worry I may have rambled without addressing the essence what you are asking;->

cheers,
jamal

[PATCH iproute2] genl: print caps for all families

[Jakub Kicinski]

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.

In fact the entire version check is not strictly necessary.
The code is not using the old attributes, so on older kernels
it will simply print nothing either way.

Families can't use flags for random things, 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

Fixes: 26328fc3933f ("Add controller support for new features exposed")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 genl/ctrl.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/genl/ctrl.c b/genl/ctrl.c
index a2d87af0ad07..8d2e944802ba 100644
--- a/genl/ctrl.c
+++ b/genl/ctrl.c
@@ -57,7 +57,7 @@ static void print_ctrl_cmd_flags(FILE *fp, __u32 fl)
 	fprintf(fp, "\n");
 }
 
-static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
+static int print_ctrl_cmds(FILE *fp, struct rtattr *arg)
 {
 	struct rtattr *tb[CTRL_ATTR_OP_MAX + 1];
 
@@ -70,7 +70,7 @@ static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
 		fprintf(fp, " ID-0x%x ",*id);
 	}
 	/* we are only gonna do this for newer version of the controller */
-	if (tb[CTRL_ATTR_OP_FLAGS] && ctrl_ver >= 0x2) {
+	if (tb[CTRL_ATTR_OP_FLAGS]) {
 		__u32 *fl = RTA_DATA(tb[CTRL_ATTR_OP_FLAGS]);
 		print_ctrl_cmd_flags(fp, *fl);
 	}
@@ -78,7 +78,7 @@ 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)
+static int print_ctrl_grp(FILE *fp, struct rtattr *arg)
 {
 	struct rtattr *tb[CTRL_ATTR_MCAST_GRP_MAX + 1];
 
@@ -109,7 +109,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
 	int len = n->nlmsg_len;
 	struct rtattr *attrs;
 	FILE *fp = (FILE *) arg;
-	__u32 ctrl_v = 0x1;
 
 	if (n->nlmsg_type !=  GENL_ID_CTRL) {
 		fprintf(stderr, "Not a controller message, nlmsg_len=%d "
@@ -148,7 +147,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
 	if (tb[CTRL_ATTR_VERSION]) {
 		__u32 *v = RTA_DATA(tb[CTRL_ATTR_VERSION]);
 		fprintf(fp, " Version: 0x%x ",*v);
-		ctrl_v = *v;
 	}
 	if (tb[CTRL_ATTR_HDRSIZE]) {
 		__u32 *h = RTA_DATA(tb[CTRL_ATTR_HDRSIZE]);
@@ -198,7 +196,7 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
 		for (i = 0; i < GENL_MAX_FAM_OPS; i++) {
 			if (tb2[i]) {
 				fprintf(fp, "\t\t#%d: ", i);
-				if (0 > print_ctrl_cmds(fp, tb2[i], ctrl_v)) {
+				if (0 > print_ctrl_cmds(fp, tb2[i])) {
 					fprintf(fp, "Error printing command\n");
 				}
 				/* for next command */
@@ -221,7 +219,7 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
 		for (i = 0; i < GENL_MAX_FAM_GRPS; i++) {
 			if (tb2[i]) {
 				fprintf(fp, "\t\t#%d: ", i);
-				if (0 > print_ctrl_grp(fp, tb2[i], ctrl_v))
+				if (0 > print_ctrl_grp(fp, tb2[i]))
 					fprintf(fp, "Error printing group\n");
 				/* for next group */
 				fprintf(fp,"\n");
-- 
2.39.2

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

[Jamal Hadi Salim]

On Fri, Feb 24, 2023 at 7:38 PM Jakub Kicinski <kuba@kernel.org> 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.
>
> In fact the entire version check is not strictly necessary.
> The code is not using the old attributes, so on older kernels
> it will simply print nothing either way.
>
> Families can't use flags for random things, 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
>
> Fixes: 26328fc3933f ("Add controller support for new features exposed")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  genl/ctrl.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/genl/ctrl.c b/genl/ctrl.c
> index a2d87af0ad07..8d2e944802ba 100644
> --- a/genl/ctrl.c
> +++ b/genl/ctrl.c
> @@ -57,7 +57,7 @@ static void print_ctrl_cmd_flags(FILE *fp, __u32 fl)
>         fprintf(fp, "\n");
>  }
>
> -static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
> +static int print_ctrl_cmds(FILE *fp, struct rtattr *arg)
>  {
>         struct rtattr *tb[CTRL_ATTR_OP_MAX + 1];
>
> @@ -70,7 +70,7 @@ static int print_ctrl_cmds(FILE *fp, struct rtattr *arg, __u32 ctrl_ver)
>                 fprintf(fp, " ID-0x%x ",*id);
>         }
>         /* we are only gonna do this for newer version of the controller */
> -       if (tb[CTRL_ATTR_OP_FLAGS] && ctrl_ver >= 0x2) {
> +       if (tb[CTRL_ATTR_OP_FLAGS]) {
>                 __u32 *fl = RTA_DATA(tb[CTRL_ATTR_OP_FLAGS]);
>                 print_ctrl_cmd_flags(fp, *fl);
>         }
> @@ -78,7 +78,7 @@ 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)
> +static int print_ctrl_grp(FILE *fp, struct rtattr *arg)
>  {
>         struct rtattr *tb[CTRL_ATTR_MCAST_GRP_MAX + 1];
>
> @@ -109,7 +109,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>         int len = n->nlmsg_len;
>         struct rtattr *attrs;
>         FILE *fp = (FILE *) arg;
> -       __u32 ctrl_v = 0x1;
>
>         if (n->nlmsg_type !=  GENL_ID_CTRL) {
>                 fprintf(stderr, "Not a controller message, nlmsg_len=%d "
> @@ -148,7 +147,6 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>         if (tb[CTRL_ATTR_VERSION]) {
>                 __u32 *v = RTA_DATA(tb[CTRL_ATTR_VERSION]);
>                 fprintf(fp, " Version: 0x%x ",*v);
> -               ctrl_v = *v;
>         }
>         if (tb[CTRL_ATTR_HDRSIZE]) {
>                 __u32 *h = RTA_DATA(tb[CTRL_ATTR_HDRSIZE]);
> @@ -198,7 +196,7 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>                 for (i = 0; i < GENL_MAX_FAM_OPS; i++) {
>                         if (tb2[i]) {
>                                 fprintf(fp, "\t\t#%d: ", i);
> -                               if (0 > print_ctrl_cmds(fp, tb2[i], ctrl_v)) {
> +                               if (0 > print_ctrl_cmds(fp, tb2[i])) {
>                                         fprintf(fp, "Error printing command\n");
>                                 }
>                                 /* for next command */
> @@ -221,7 +219,7 @@ static int print_ctrl(struct rtnl_ctrl_data *ctrl,
>                 for (i = 0; i < GENL_MAX_FAM_GRPS; i++) {
>                         if (tb2[i]) {
>                                 fprintf(fp, "\t\t#%d: ", i);
> -                               if (0 > print_ctrl_grp(fp, tb2[i], ctrl_v))
> +                               if (0 > print_ctrl_grp(fp, tb2[i]))
>                                         fprintf(fp, "Error printing group\n");
>                                 /* for next group */
>                                 fprintf(fp,"\n");
> --
> 2.39.2
>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to iproute2/iproute2.git (main)
by Stephen Hemminger <stephen@networkplumber.org>:

On Fri, 24 Feb 2023 16:37:54 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - [iproute2] genl: print caps for all families
    https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=2a98bc13169b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html