[PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[Bjorn Andersson]

Parse and pass IFLA_RMNET_FLAGS to the kernel, to allow changing the
flags from the default of ingress-aggregate only.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- s/ifla_vlan_flags/ifla_rmnet_flags/ in print_opt

 ip/iplink_rmnet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
index 1d16440c6900..a847c838def2 100644
--- a/ip/iplink_rmnet.c
+++ b/ip/iplink_rmnet.c
@@ -16,6 +16,10 @@ static void print_explain(FILE *f)
 {
 	fprintf(f,
 		"Usage: ... rmnet mux_id MUXID\n"
+		"                 [ingress-deaggregation]\n"
+		"                 [ingress-commands]\n"
+		"                 [ingress-chksumv4]\n"
+		"                 [egress-chksumv4]\n"
 		"\n"
 		"MUXID := 1-254\n"
 	);
@@ -29,6 +33,7 @@ static void explain(void)
 static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
 			   struct nlmsghdr *n)
 {
+	struct ifla_rmnet_flags flags = { };
 	__u16 mux_id;
 
 	while (argc > 0) {
@@ -37,6 +42,18 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
 			if (get_u16(&mux_id, *argv, 0))
 				invarg("mux_id is invalid", *argv);
 			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
+		} else if (matches(*argv, "ingress-deaggregation") == 0) {
+			flags.mask = ~0;
+			flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
+		} else if (matches(*argv, "ingress-commands") == 0) {
+			flags.mask = ~0;
+			flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
+		} else if (matches(*argv, "ingress-chksumv4") == 0) {
+			flags.mask = ~0;
+			flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
+		} else if (matches(*argv, "egress-chksumv4") == 0) {
+			flags.mask = ~0;
+			flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
 		} else if (matches(*argv, "help") == 0) {
 			explain();
 			return -1;
@@ -48,11 +65,28 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
 		argc--, argv++;
 	}
 
+	if (flags.mask)
+		addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
+
 	return 0;
 }
 
+static void rmnet_print_flags(FILE *fp, __u32 flags)
+{
+	if (flags & RMNET_FLAGS_INGRESS_DEAGGREGATION)
+		print_string(PRINT_ANY, NULL, "%s ", "ingress-deaggregation");
+	if (flags & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
+		print_string(PRINT_ANY, NULL, "%s ", "ingress-commands");
+	if (flags & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
+		print_string(PRINT_ANY, NULL, "%s ", "ingress-chksumv4");
+	if (flags & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
+		print_string(PRINT_ANY, NULL, "%s ", "egress-cksumv4");
+}
+
 static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
+	struct ifla_rmnet_flags *flags;
+
 	if (!tb)
 		return;
 
@@ -64,6 +98,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		   "mux_id",
 		   "mux_id %u ",
 		   rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
+
+	if (tb[IFLA_RMNET_FLAGS]) {
+		if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
+			return;
+		flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
+
+		rmnet_print_flags(f, flags->flags);
+	}
 }
 
 static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
-- 
2.28.0

Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[Alex Elder]

On 3/15/21 10:46 AM, Bjorn Andersson wrote:
> Parse and pass IFLA_RMNET_FLAGS to the kernel, to allow changing the
> flags from the default of ingress-aggregate only.

To be clear, this default is implemented in the kernel RMNet
driver, not in "iproute2".  And it is ingress deaggregation
(unpacking of aggregated packets from a buffer), not aggregation
(which would be supplying a buffer of aggregated packets to the
hardware).

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

I have some suggestions on your help text (and flag names).
The code looks good to me otherwise.  I trust you've
confirmed the RMNet driver uses the flags exactly as
you intend when they're provided this way.

					-Alex
> ---
> 
> Changes since v1:
> - s/ifla_vlan_flags/ifla_rmnet_flags/ in print_opt
> 
>   ip/iplink_rmnet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
> index 1d16440c6900..a847c838def2 100644
> --- a/ip/iplink_rmnet.c
> +++ b/ip/iplink_rmnet.c
> @@ -16,6 +16,10 @@ static void print_explain(FILE *f)
>   {
>   	fprintf(f,
>   		"Usage: ... rmnet mux_id MUXID\n"
> +		"                 [ingress-deaggregation]\n"
> +		"                 [ingress-commands]\n"
> +		"                 [ingress-chksumv4]\n"
> +		"                 [egress-chksumv4]\n"

Other help output (in print_explain()) put spaces after
the '[' and before the ']'; so you'd be better to stay
consistent with that.

And I know the name is based on the C symbol, but I think
you should follow the convention that seems to be used for
all others, and use "csum" to mean checksum.

Also it's not clear what the "v4" means.  I'm not sure I
like this suggestion, but...  It comes from QMAP version 4,
as opposed to QMAP version 5, so maybe use "csum-qmap4"
in place of "csumv4?"

Is there any way to disable ingress deaggregation?  Since
it's on by default, you might want to use a "[ on | off ]"
type option for that case (or all of them for that matter).
Otherwise, the deaggregation parameter doesn't really help
anything.

>   		"\n"
>   		"MUXID := 1-254\n"
>   	);
> @@ -29,6 +33,7 @@ static void explain(void)
>   static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   			   struct nlmsghdr *n)
>   {
> +	struct ifla_rmnet_flags flags = { };
>   	__u16 mux_id;

Do you know why this is __u16?  Is it because it's exposed
to user space?  Not a problem... just curious.

>   	while (argc > 0) {
> @@ -37,6 +42,18 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   			if (get_u16(&mux_id, *argv, 0))
>   				invarg("mux_id is invalid", *argv);
>   			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
> +		} else if (matches(*argv, "ingress-deaggregation") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +		} else if (matches(*argv, "ingress-commands") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +		} else if (matches(*argv, "ingress-chksumv4") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +		} else if (matches(*argv, "egress-chksumv4") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
>   		} else if (matches(*argv, "help") == 0) {
>   			explain();
>   			return -1;
> @@ -48,11 +65,28 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   		argc--, argv++;
>   	}
>   
> +	if (flags.mask)
> +		addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
> +
>   	return 0;
>   }
>   
> +static void rmnet_print_flags(FILE *fp, __u32 flags)
> +{
> +	if (flags & RMNET_FLAGS_INGRESS_DEAGGREGATION)
> +		print_string(PRINT_ANY, NULL, "%s ", "ingress-deaggregation");
> +	if (flags & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
> +		print_string(PRINT_ANY, NULL, "%s ", "ingress-commands");
> +	if (flags & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
> +		print_string(PRINT_ANY, NULL, "%s ", "ingress-chksumv4");
> +	if (flags & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
> +		print_string(PRINT_ANY, NULL, "%s ", "egress-cksumv4");
> +}
> +
>   static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>   {
> +	struct ifla_rmnet_flags *flags;
> +
>   	if (!tb)
>   		return;
>   
> @@ -64,6 +98,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>   		   "mux_id",
>   		   "mux_id %u ",
>   		   rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
> +
> +	if (tb[IFLA_RMNET_FLAGS]) {
> +		if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
> +			return;
> +		flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
> +
> +		rmnet_print_flags(f, flags->flags);
> +	}
>   }
>   
>   static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
> 

Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[Bjorn Andersson]

On Mon 15 Mar 11:16 CDT 2021, Alex Elder wrote:

> On 3/15/21 10:46 AM, Bjorn Andersson wrote:
> > Parse and pass IFLA_RMNET_FLAGS to the kernel, to allow changing the
> > flags from the default of ingress-aggregate only.
> 
> To be clear, this default is implemented in the kernel RMNet
> driver, not in "iproute2".  And it is ingress deaggregation
> (unpacking of aggregated packets from a buffer), not aggregation
> (which would be supplying a buffer of aggregated packets to the
> hardware).
> 

Thanks, I'll update the commit message to clarify this point.

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> I have some suggestions on your help text (and flag names).
> The code looks good to me otherwise.  I trust you've
> confirmed the RMNet driver uses the flags exactly as
> you intend when they're provided this way.
> 

I've confirmed that it flips the bits in the rmnet driver and that
flipping the right bit(s) my laptop starts deaggregating messages.

> 					-Alex
> > ---
> > 
> > Changes since v1:
> > - s/ifla_vlan_flags/ifla_rmnet_flags/ in print_opt
> > 
> >   ip/iplink_rmnet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> > 
> > diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
> > index 1d16440c6900..a847c838def2 100644
> > --- a/ip/iplink_rmnet.c
> > +++ b/ip/iplink_rmnet.c
> > @@ -16,6 +16,10 @@ static void print_explain(FILE *f)
> >   {
> >   	fprintf(f,
> >   		"Usage: ... rmnet mux_id MUXID\n"
> > +		"                 [ingress-deaggregation]\n"
> > +		"                 [ingress-commands]\n"
> > +		"                 [ingress-chksumv4]\n"
> > +		"                 [egress-chksumv4]\n"
> 
> Other help output (in print_explain()) put spaces after
> the '[' and before the ']'; so you'd be better to stay
> consistent with that.
> 

I had missed that, thanks.

> And I know the name is based on the C symbol, but I think
> you should follow the convention that seems to be used for
> all others, and use "csum" to mean checksum.
> 
> Also it's not clear what the "v4" means.  I'm not sure I
> like this suggestion, but...  It comes from QMAP version 4,
> as opposed to QMAP version 5, so maybe use "csum-qmap4"
> in place of "csumv4?"
> 

Make sense, I'll update this to ingress-csum-qmap4 etc.

> Is there any way to disable ingress deaggregation?  Since
> it's on by default, you might want to use a "[ on | off ]"
> type option for that case (or all of them for that matter).
> Otherwise, the deaggregation parameter doesn't really help
> anything.
> 

Unfortunately, in contrast to other implementers of IFLAG_x_FLAGS we
find the following snippet in the rmnet driver:

  data_format = flags->flags & flags->mask;

So rather than flags->mask specifying which bits to update, it masks
flags->flags and whatever is left is your new flags. And given that this
is non-standard, the iplink flow doesn't support read/modify/write on
the tool side.

As such it's not possible to toggle individual bits, you always pass the
new flags. So the way to disable a particular feature, is to issue an
change link request without the particular feature specified.

> >   		"\n"
> >   		"MUXID := 1-254\n"
> >   	);
> > @@ -29,6 +33,7 @@ static void explain(void)
> >   static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
> >   			   struct nlmsghdr *n)
> >   {
> > +	struct ifla_rmnet_flags flags = { };
> >   	__u16 mux_id;
> 
> Do you know why this is __u16?  Is it because it's exposed
> to user space?  Not a problem... just curious.
> 

This seems to be the data type used throughout the project to denote
16 bit unsigned integers, so it seems to me that Daniele just followed
the coding standard of the project here - and it's defined in the kernel
to be a 16 bit unsigned integer...

Regards,
Bjorn

> >   	while (argc > 0) {
> > @@ -37,6 +42,18 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
> >   			if (get_u16(&mux_id, *argv, 0))
> >   				invarg("mux_id is invalid", *argv);
> >   			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
> > +		} else if (matches(*argv, "ingress-deaggregation") == 0) {
> > +			flags.mask = ~0;
> > +			flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> > +		} else if (matches(*argv, "ingress-commands") == 0) {
> > +			flags.mask = ~0;
> > +			flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> > +		} else if (matches(*argv, "ingress-chksumv4") == 0) {
> > +			flags.mask = ~0;
> > +			flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> > +		} else if (matches(*argv, "egress-chksumv4") == 0) {
> > +			flags.mask = ~0;
> > +			flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> >   		} else if (matches(*argv, "help") == 0) {
> >   			explain();
> >   			return -1;
> > @@ -48,11 +65,28 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
> >   		argc--, argv++;
> >   	}
> > +	if (flags.mask)
> > +		addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
> > +
> >   	return 0;
> >   }
> > +static void rmnet_print_flags(FILE *fp, __u32 flags)
> > +{
> > +	if (flags & RMNET_FLAGS_INGRESS_DEAGGREGATION)
> > +		print_string(PRINT_ANY, NULL, "%s ", "ingress-deaggregation");
> > +	if (flags & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
> > +		print_string(PRINT_ANY, NULL, "%s ", "ingress-commands");
> > +	if (flags & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
> > +		print_string(PRINT_ANY, NULL, "%s ", "ingress-chksumv4");
> > +	if (flags & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
> > +		print_string(PRINT_ANY, NULL, "%s ", "egress-cksumv4");
> > +}
> > +
> >   static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >   {
> > +	struct ifla_rmnet_flags *flags;
> > +
> >   	if (!tb)
> >   		return;
> > @@ -64,6 +98,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >   		   "mux_id",
> >   		   "mux_id %u ",
> >   		   rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
> > +
> > +	if (tb[IFLA_RMNET_FLAGS]) {
> > +		if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
> > +			return;
> > +		flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
> > +
> > +		rmnet_print_flags(f, flags->flags);
> > +	}
> >   }
> >   static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
> > 
> 

[PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[Bjorn Andersson]

Extend the rmnet option parser to allow enabling and disabling
IFLA_RMNET_FLAGS using ip link and add the flags to the pint_op to allow
inspecting the current settings.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Landed ABI change to allow setting/clearing individual bits
- Changed parser to take on/off arguments
- Added the new v5 chksum bits
- Made print_flags fancier, with some inspiration from iplink_vlan

 ip/iplink_rmnet.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
index 1d16440c6900..f629ca9976d9 100644
--- a/ip/iplink_rmnet.c
+++ b/ip/iplink_rmnet.c
@@ -16,6 +16,12 @@ static void print_explain(FILE *f)
 {
 	fprintf(f,
 		"Usage: ... rmnet mux_id MUXID\n"
+		"                 [ingress-deaggregation { on | off } ]\n"
+		"                 [ingress-commands { on | off } ]\n"
+		"                 [ingress-chksumv4 { on | off } ]\n"
+		"                 [ingress-chksumv5 { on | off } ]\n"
+		"                 [egress-chksumv4 { on | off } ]\n"
+		"                 [egress-chksumv5 { on | off } ]\n"
 		"\n"
 		"MUXID := 1-254\n"
 	);
@@ -26,9 +32,16 @@ static void explain(void)
 	print_explain(stderr);
 }
 
+static int on_off(const char *msg, const char *arg)
+{
+	fprintf(stderr, "Error: argument of \"%s\" must be \"on\" or \"off\", not \"%s\"\n", msg, arg);
+	return -1;
+}
+
 static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
 			   struct nlmsghdr *n)
 {
+	struct ifla_rmnet_flags flags = { };
 	__u16 mux_id;
 
 	while (argc > 0) {
@@ -37,6 +50,60 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
 			if (get_u16(&mux_id, *argv, 0))
 				invarg("mux_id is invalid", *argv);
 			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
+		} else if (matches(*argv, "ingress-deaggregation") == 0) {
+			NEXT_ARG();
+			flags.mask |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
+			if (strcmp(*argv, "on") == 0)
+				flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
+			else if (strcmp(*argv, "off") == 0)
+				flags.flags &= ~RMNET_FLAGS_INGRESS_DEAGGREGATION;
+			else
+				return on_off("ingress-deaggregation", *argv);
+		} else if (matches(*argv, "ingress-commands") == 0) {
+			NEXT_ARG();
+			flags.mask |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
+			if (strcmp(*argv, "on") == 0)
+				flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
+			else if (strcmp(*argv, "off") == 0)
+				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_COMMANDS;
+			else
+				return on_off("ingress-commands", *argv);
+		} else if (matches(*argv, "ingress-chksumv4") == 0) {
+			NEXT_ARG();
+			flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
+			if (strcmp(*argv, "on") == 0)
+				flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
+			else if (strcmp(*argv, "off") == 0)
+				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
+			else
+				return on_off("ingress-chksumv4", *argv);
+		} else if (matches(*argv, "ingress-chksumv5") == 0) {
+			NEXT_ARG();
+			flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
+			if (strcmp(*argv, "on") == 0)
+				flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
+			else if (strcmp(*argv, "off") == 0)
+				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
+			else
+				return on_off("ingress-chksumv5", *argv);
+		} else if (matches(*argv, "egress-chksumv4") == 0) {
+			NEXT_ARG();
+			flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
+			if (strcmp(*argv, "on") == 0)
+				flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
+			else if (strcmp(*argv, "off") == 0)
+				flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
+			else
+				return on_off("egress-chksumv4", *argv);
+		} else if (matches(*argv, "egress-chksumv5") == 0) {
+			NEXT_ARG();
+			flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
+			if (strcmp(*argv, "on") == 0)
+				flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
+			else if (strcmp(*argv, "off") == 0)
+				flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
+			else
+				return on_off("egress-chksumv5", *argv);
 		} else if (matches(*argv, "help") == 0) {
 			explain();
 			return -1;
@@ -48,11 +115,33 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
 		argc--, argv++;
 	}
 
+	if (flags.mask)
+		addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
+
 	return 0;
 }
 
+static void rmnet_print_flags(FILE *fp, __u32 flags)
+{
+	open_json_array(PRINT_ANY, is_json_context() ? "flags" : "<");
+#define _PF(f, s) if (flags & RMNET_FLAGS_##f) {			\
+		flags &= ~RMNET_FLAGS_##f;				\
+		print_string(PRINT_ANY, NULL, flags ? "%s," : "%s", s); \
+	}
+	_PF(INGRESS_DEAGGREGATION, "ingress-deaggregation");
+	_PF(INGRESS_MAP_COMMANDS, "ingress-commands");
+	_PF(INGRESS_MAP_CKSUMV4, "ingress-chksumv4");
+	_PF(INGRESS_MAP_CKSUMV5, "ingress-chksumv5");
+	_PF(EGRESS_MAP_CKSUMV4, "egress-chksumv4");
+	_PF(EGRESS_MAP_CKSUMV5, "egress-chksumv5");
+#undef _PF
+	close_json_array(PRINT_ANY, "> ");
+}
+
 static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
+	struct ifla_vlan_flags *flags;
+
 	if (!tb)
 		return;
 
@@ -64,6 +153,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		   "mux_id",
 		   "mux_id %u ",
 		   rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
+
+	if (tb[IFLA_RMNET_FLAGS]) {
+		if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
+			return;
+		flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
+
+		rmnet_print_flags(f, flags->flags);
+	}
 }
 
 static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
-- 
2.33.0

Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[David Ahern]

On 9/23/21 9:33 PM, Bjorn Andersson wrote:
> diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
> index 1d16440c6900..f629ca9976d9 100644
> --- a/ip/iplink_rmnet.c
> +++ b/ip/iplink_rmnet.c
> @@ -16,6 +16,12 @@ static void print_explain(FILE *f)
>  {
>  	fprintf(f,
>  		"Usage: ... rmnet mux_id MUXID\n"
> +		"                 [ingress-deaggregation { on | off } ]\n"
> +		"                 [ingress-commands { on | off } ]\n"
> +		"                 [ingress-chksumv4 { on | off } ]\n"
> +		"                 [ingress-chksumv5 { on | off } ]\n"
> +		"                 [egress-chksumv4 { on | off } ]\n"
> +		"                 [egress-chksumv5 { on | off } ]\n"
>  		"\n"
>  		"MUXID := 1-254\n"
>  	);
> @@ -26,9 +32,16 @@ static void explain(void)
>  	print_explain(stderr);
>  }
>  
> +static int on_off(const char *msg, const char *arg)
> +{
> +	fprintf(stderr, "Error: argument of \"%s\" must be \"on\" or \"off\", not \"%s\"\n", msg, arg);
> +	return -1;
> +}
> +
>  static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>  			   struct nlmsghdr *n)
>  {
> +	struct ifla_rmnet_flags flags = { };
>  	__u16 mux_id;
>  
>  	while (argc > 0) {
> @@ -37,6 +50,60 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>  			if (get_u16(&mux_id, *argv, 0))
>  				invarg("mux_id is invalid", *argv);
>  			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
> +		} else if (matches(*argv, "ingress-deaggregation") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +			else
> +				return on_off("ingress-deaggregation", *argv);
> +		} else if (matches(*argv, "ingress-commands") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +			else
> +				return on_off("ingress-commands", *argv);
> +		} else if (matches(*argv, "ingress-chksumv4") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +			else
> +				return on_off("ingress-chksumv4", *argv);
> +		} else if (matches(*argv, "ingress-chksumv5") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> +			else
> +				return on_off("ingress-chksumv5", *argv);
> +		} else if (matches(*argv, "egress-chksumv4") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +			else
> +				return on_off("egress-chksumv4", *argv);
> +		} else if (matches(*argv, "egress-chksumv5") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +			else
> +				return on_off("egress-chksumv5", *argv);
>  		} else if (matches(*argv, "help") == 0) {
>  			explain();
>  			return -1;

use strcmp for new options. Also, please use 'csum' instead of 'chksum'
in the names. csum is already widely used in ip commands.

Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[Alex Elder]

On 9/27/21 8:48 AM, David Ahern wrote:
>>   		} else if (matches(*argv, "help") == 0) {
>>   			explain();
>>   			return -1;
> use strcmp for new options. Also, please use 'csum' instead of 'chksum'
> in the names. csum is already widely used in ip commands.

On the csum remark, I agree completely.

On the other:  Are you saying to use strcmp() instead of
matches()?

That seems strange to me because matches() is used *much*
more often than strcmp(), and handles an empty *argv
differently.

I don't disagree with your suggested change, but upon
looking at the other code it surprises me a bit.  Can
you provide a little more explanation?  If you mean
something else, please clarify.  Thanks.

					-Alex

Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[David Ahern]

On 9/27/21 4:44 PM, Alex Elder wrote:
> On 9/27/21 8:48 AM, David Ahern wrote:
>>>           } else if (matches(*argv, "help") == 0) {
>>>               explain();
>>>               return -1;
>> use strcmp for new options. Also, please use 'csum' instead of 'chksum'
>> in the names. csum is already widely used in ip commands.
> 
> On the csum remark, I agree completely.
> 
> On the other:  Are you saying to use strcmp() instead of
> matches()?
> 
> That seems strange to me because matches() is used *much*
> more often than strcmp(), and handles an empty *argv
> differently.

matches is way beyond its usefulness. Its use is now deprecated. It just
leads to confusing command lines and problems maintaining which option
hits the matches.

> 
> I don't disagree with your suggested change, but upon
> looking at the other code it surprises me a bit.  Can
> you provide a little more explanation?  If you mean
> something else, please clarify.  Thanks.
> 
>                     -Alex

Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS

[Alex Elder]

On 9/23/21 10:33 PM, Bjorn Andersson wrote:
> Extend the rmnet option parser to allow enabling and disabling
> IFLA_RMNET_FLAGS using ip link and add the flags to the pint_op to allow
> inspecting the current settings.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

I think what you're doing looks OK to me, but I know
David had suggestions so I expect I'll get another shot
at review...

I have a couple very minor comments, and they're basically
unrelated.  So I guess take it or leave it.

					-Alex

> ---
> 
> Changes since v1:
> - Landed ABI change to allow setting/clearing individual bits
> - Changed parser to take on/off arguments
> - Added the new v5 chksum bits
> - Made print_flags fancier, with some inspiration from iplink_vlan
> 
>   ip/iplink_rmnet.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 97 insertions(+)
> 
> diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
> index 1d16440c6900..f629ca9976d9 100644
> --- a/ip/iplink_rmnet.c
> +++ b/ip/iplink_rmnet.c
> @@ -16,6 +16,12 @@ static void print_explain(FILE *f)
>   {
>   	fprintf(f,
>   		"Usage: ... rmnet mux_id MUXID\n"
> +		"                 [ingress-deaggregation { on | off } ]\n"
> +		"                 [ingress-commands { on | off } ]\n"
> +		"                 [ingress-chksumv4 { on | off } ]\n"
> +		"                 [ingress-chksumv5 { on | off } ]\n"
> +		"                 [egress-chksumv4 { on | off } ]\n"
> +		"                 [egress-chksumv5 { on | off } ]\n"

Looking at other usage messages, it appears that the dash
rather than underscore is preferable (though not universal).
I.e., I think you did this right.

>   		"\n"
>   		"MUXID := 1-254\n"
>   	);
> @@ -26,9 +32,16 @@ static void explain(void)
>   	print_explain(stderr);
>   }
>   
> +static int on_off(const char *msg, const char *arg)
> +{
> +	fprintf(stderr, "Error: argument of \"%s\" must be \"on\" or \"off\", not \"%s\"\n", msg, arg);

It would be nice if this function could be defined centrally rather
than repeated.  Maybe defined in "lib/utils.c", declared in
"include/utils.h"?  Not your problem but if you were so moved,
that could be done in a separate patch.

> +	return -1;
> +}
> +
>   static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   			   struct nlmsghdr *n)
>   {
> +	struct ifla_rmnet_flags flags = { };
>   	__u16 mux_id;
>   
>   	while (argc > 0) {
> @@ -37,6 +50,60 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   			if (get_u16(&mux_id, *argv, 0))
>   				invarg("mux_id is invalid", *argv);
>   			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
> +		} else if (matches(*argv, "ingress-deaggregation") == 0) {
> +			NEXT_ARG();

With these repeated blocks of code my instinct is to suggest
creating a common called function, but it gets a little ugly.
What I sketch below precludes using NEXT_ARG() where it's
needed.

static int matches_on_off(ifla_rmnet_flags *flags, u32 mask,
			  const char *name, char *arg)
{
	if (matches(arg, name))
		return 0;

	if (strcmp(arg, "on") == 0)
		flags->flag |= mask;
	else if (strcmp(arg, "off") == 0) {
		flags->flag &= ~mask;
	else
		return on_off(name, arg);

	flags->mask |= mask;

	return 1;
}

Then use:

	if (matches_on_off(&flags, RMNET_FLAGS_INGRESS_DEAGGREGATION,
			   "ingress-deaggregation", *argv) < 0)
		return -1;
	else if (matches_on_off(...))
		. . .

So I'm not sure I like how this looks, and once again,
it isn't really necessary for what you're doing here...

> +			flags.mask |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +			else
> +				return on_off("ingress-deaggregation", *argv);
> +		} else if (matches(*argv, "ingress-commands") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +			else
> +				return on_off("ingress-commands", *argv);
> +		} else if (matches(*argv, "ingress-chksumv4") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +			else
> +				return on_off("ingress-chksumv4", *argv);
> +		} else if (matches(*argv, "ingress-chksumv5") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> +			else
> +				return on_off("ingress-chksumv5", *argv);
> +		} else if (matches(*argv, "egress-chksumv4") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +			else
> +				return on_off("egress-chksumv4", *argv);
> +		} else if (matches(*argv, "egress-chksumv5") == 0) {
> +			NEXT_ARG();
> +			flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +			if (strcmp(*argv, "on") == 0)
> +				flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +			else if (strcmp(*argv, "off") == 0)
> +				flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +			else
> +				return on_off("egress-chksumv5", *argv);
>   		} else if (matches(*argv, "help") == 0) {
>   			explain();
>   			return -1;
> @@ -48,11 +115,33 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   		argc--, argv++;
>   	}
>   
> +	if (flags.mask)
> +		addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
> +
>   	return 0;
>   }
>   
> +static void rmnet_print_flags(FILE *fp, __u32 flags)
> +{
> +	open_json_array(PRINT_ANY, is_json_context() ? "flags" : "<");
> +#define _PF(f, s) if (flags & RMNET_FLAGS_##f) {			\
> +		flags &= ~RMNET_FLAGS_##f;				\
> +		print_string(PRINT_ANY, NULL, flags ? "%s," : "%s", s); \
> +	}
> +	_PF(INGRESS_DEAGGREGATION, "ingress-deaggregation");
> +	_PF(INGRESS_MAP_COMMANDS, "ingress-commands");
> +	_PF(INGRESS_MAP_CKSUMV4, "ingress-chksumv4");
> +	_PF(INGRESS_MAP_CKSUMV5, "ingress-chksumv5");
> +	_PF(EGRESS_MAP_CKSUMV4, "egress-chksumv4");
> +	_PF(EGRESS_MAP_CKSUMV5, "egress-chksumv5");
> +#undef _PF
> +	close_json_array(PRINT_ANY, "> ");
> +}
> +
>   static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>   {
> +	struct ifla_vlan_flags *flags;
> +
>   	if (!tb)
>   		return;
>   
> @@ -64,6 +153,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>   		   "mux_id",
>   		   "mux_id %u ",
>   		   rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
> +
> +	if (tb[IFLA_RMNET_FLAGS]) {
> +		if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
> +			return;
> +		flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
> +
> +		rmnet_print_flags(f, flags->flags);
> +	}
>   }
>   
>   static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
>