netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] tc: qdisc: filter qdisc's by parent/handle specification
@ 2020-06-16  6:39 Anton Danilov
  2020-06-18 22:17 ` Stephen Hemminger
  0 siblings, 1 reply; 5+ messages in thread
From: Anton Danilov @ 2020-06-16  6:39 UTC (permalink / raw)
  To: netdev, stephen; +Cc: Anton Danilov

There wasn't a way to get a qdisc info by handle or parent, only full
dump of qdisc's with following grep/sed usage.

The 'qdisc get' command have been added.

tc qdisc { show | get } [ dev STRING ] [ QDISC_ID ] [ invisible ]
  QDISC_ID := { root | ingress | handle QHANDLE | parent CLASSID }

This change doesn't require any changes in the kernel.

Signed-off-by: Anton Danilov <littlesmilingcloud@gmail.com>
---

changes since v1:
  coding style fixes

---
 man/man8/tc.8 |   8 +++-
 tc/tc_qdisc.c | 111 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 84 insertions(+), 35 deletions(-)

diff --git a/man/man8/tc.8 b/man/man8/tc.8
index e8e0cd0f..8753088f 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -77,9 +77,13 @@ tc \- show / manipulate traffic control settings
 .B tc
 .RI "[ " OPTIONS " ]"
 .RI "[ " FORMAT " ]"
-.B qdisc show [ dev
+.B qdisc { show | get } [ dev
 \fIDEV\fR
-.B ]
+.B ] [ root | ingress | handle
+\fIQHANDLE\fR
+.B | parent
+\fICLASSID\fR
+.B ] [ invisible ]
 .P
 .B tc
 .RI "[ " OPTIONS " ]"
diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index 181fe2f0..af2dd1c8 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -35,11 +35,12 @@ static int usage(void)
 		"       [ ingress_block BLOCK_INDEX ] [ egress_block BLOCK_INDEX ]\n"
 		"       [ [ QDISC_KIND ] [ help | OPTIONS ] ]\n"
 		"\n"
-		"       tc qdisc show [ dev STRING ] [ ingress | clsact ] [ invisible ]\n"
+		"       tc qdisc { show | get } [ dev STRING ] [ QDISC_ID ] [ invisible ]\n"
 		"Where:\n"
 		"QDISC_KIND := { [p|b]fifo | tbf | prio | cbq | red | etc. }\n"
 		"OPTIONS := ... try tc qdisc add <desired QDISC_KIND> help\n"
-		"STAB_OPTIONS := ... try tc qdisc add stab help\n");
+		"STAB_OPTIONS := ... try tc qdisc add stab help\n"
+		"QDISC_ID := { root | ingress | handle QHANDLE | parent CLASSID }\n");
 	return -1;
 }
 
@@ -212,6 +213,8 @@ static int tc_qdisc_modify(int cmd, unsigned int flags, int argc, char **argv)
 }
 
 static int filter_ifindex;
+static __u32 filter_parent;
+static __u32 filter_handle;
 
 int print_qdisc(struct nlmsghdr *n, void *arg)
 {
@@ -235,6 +238,12 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
 	if (filter_ifindex && filter_ifindex != t->tcm_ifindex)
 		return 0;
 
+	if (filter_handle && filter_handle != t->tcm_handle)
+		return 0;
+
+	if (filter_parent && filter_parent != t->tcm_parent)
+		return 0;
+
 	parse_rtattr_flags(tb, TCA_MAX, TCA_RTA(t), len, NLA_F_NESTED);
 
 	if (tb[TCA_KIND] == NULL) {
@@ -344,21 +353,70 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
 
 static int tc_qdisc_list(int argc, char **argv)
 {
-	struct tcmsg t = { .tcm_family = AF_UNSPEC };
+	struct {
+		struct nlmsghdr n;
+		struct tcmsg t;
+		char buf[256];
+	} req = {
+		.n.nlmsg_type = RTM_GETQDISC,
+		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
+		.t.tcm_family = AF_UNSPEC,
+	};
+
 	char d[IFNAMSIZ] = {};
+	bool arg_error = false;
 	bool dump_invisible = false;
+	__u32 handle;
 
-	while (argc > 0) {
+	while (argc > 0 && !arg_error) {
 		if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
 			strncpy(d, *argv, sizeof(d)-1);
+		} else if (strcmp(*argv, "root") == 0) {
+			if (filter_parent || filter_handle) {
+				fprintf(stderr,
+					"only one of parent/handle/root/ingress can be specified\n");
+				arg_error = true;
+				break;
+			}
+			filter_parent = TC_H_ROOT;
 		} else if (strcmp(*argv, "ingress") == 0 ||
 			   strcmp(*argv, "clsact") == 0) {
-			if (t.tcm_parent) {
-				fprintf(stderr, "Duplicate parent ID\n");
-				usage();
+			if (filter_parent || filter_handle) {
+				fprintf(stderr,
+				    "only one of parent/handle/root/ingress can be specified\n");
+				arg_error = true;
+				break;
 			}
-			t.tcm_parent = TC_H_INGRESS;
+			filter_parent = TC_H_INGRESS;
+		} else if (matches(*argv, "parent") == 0) {
+			if (filter_parent || filter_handle) {
+				fprintf(stderr,
+				    "only one of parent/handle/root/ingress can be specified\n");
+				arg_error = true;
+				break;
+			}
+			NEXT_ARG();
+			if (get_tc_classid(&handle, *argv)) {
+				invarg("invalid parent ID", *argv);
+				arg_error = true;
+				break;
+			}
+			filter_parent = handle;
+		} else if (matches(*argv, "handle") == 0) {
+			if (filter_parent || filter_handle) {
+				fprintf(stderr,
+				    "only one of parent/handle/root/ingress can be specified\n");
+				arg_error = true;
+				break;
+			}
+			NEXT_ARG();
+			if (get_qdisc_handle(&handle, *argv)) {
+				invarg("invalid handle ID", *argv);
+				arg_error = true;
+				break;
+			}
+			filter_handle = handle;
 		} else if (matches(*argv, "help") == 0) {
 			usage();
 		} else if (strcmp(*argv, "invisible") == 0) {
@@ -371,35 +429,26 @@ static int tc_qdisc_list(int argc, char **argv)
 		argc--; argv++;
 	}
 
+	if (arg_error) {
+		/* argument error message should be already displayed above */
+		return -1;
+	}
+
 	ll_init_map(&rth);
 
 	if (d[0]) {
-		t.tcm_ifindex = ll_name_to_index(d);
-		if (!t.tcm_ifindex)
+		req.t.tcm_ifindex = ll_name_to_index(d);
+		if (!req.t.tcm_ifindex)
 			return -nodev(d);
-		filter_ifindex = t.tcm_ifindex;
+		filter_ifindex = req.t.tcm_ifindex;
 	}
 
 	if (dump_invisible) {
-		struct {
-			struct nlmsghdr n;
-			struct tcmsg t;
-			char buf[256];
-		} req = {
-			.n.nlmsg_type = RTM_GETQDISC,
-			.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
-		};
-
-		req.t.tcm_family = AF_UNSPEC;
-
 		addattr(&req.n, 256, TCA_DUMP_INVISIBLE);
-		if (rtnl_dump_request_n(&rth, &req.n) < 0) {
-			perror("Cannot send dump request");
-			return 1;
-		}
+	}
 
-	} else if (rtnl_dump_request(&rth, RTM_GETQDISC, &t, sizeof(t)) < 0) {
-		perror("Cannot send dump request");
+	if (rtnl_dump_request_n(&rth, &req.n) < 0) {
+		perror("Cannot send request");
 		return 1;
 	}
 
@@ -427,12 +476,8 @@ int do_qdisc(int argc, char **argv)
 		return tc_qdisc_modify(RTM_NEWQDISC, NLM_F_REPLACE, argc-1, argv+1);
 	if (matches(*argv, "delete") == 0)
 		return tc_qdisc_modify(RTM_DELQDISC, 0,  argc-1, argv+1);
-#if 0
-	if (matches(*argv, "get") == 0)
-		return tc_qdisc_get(RTM_GETQDISC, 0,  argc-1, argv+1);
-#endif
 	if (matches(*argv, "list") == 0 || matches(*argv, "show") == 0
-	    || matches(*argv, "lst") == 0)
+	    || matches(*argv, "lst") == 0 || matches(*argv, "get") == 0)
 		return tc_qdisc_list(argc-1, argv+1);
 	if (matches(*argv, "help") == 0) {
 		usage();
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] tc: qdisc: filter qdisc's by parent/handle specification
  2020-06-16  6:39 [PATCH v2] tc: qdisc: filter qdisc's by parent/handle specification Anton Danilov
@ 2020-06-18 22:17 ` Stephen Hemminger
  2020-06-24 17:03   ` Anton Danilov
  2020-07-03 15:39   ` [PATCH iproute2 v3] tc: improve the qdisc show command Anton Danilov
  0 siblings, 2 replies; 5+ messages in thread
From: Stephen Hemminger @ 2020-06-18 22:17 UTC (permalink / raw)
  To: Anton Danilov; +Cc: netdev

On Tue, 16 Jun 2020 09:39:02 +0300
Anton Danilov <littlesmilingcloud@gmail.com> wrote:

> +				fprintf(stderr,
> +					"only one of parent/handle/root/ingress can be specified\n");
> +				arg_error = true;
> +				break;

The concept is good, but it could be simplified.

You are repeating same error message several places and it is not
necessary to continue parsing after one bad argument.

See what invarg() does.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] tc: qdisc: filter qdisc's by parent/handle specification
  2020-06-18 22:17 ` Stephen Hemminger
@ 2020-06-24 17:03   ` Anton Danilov
  2020-07-03 15:39   ` [PATCH iproute2 v3] tc: improve the qdisc show command Anton Danilov
  1 sibling, 0 replies; 5+ messages in thread
From: Anton Danilov @ 2020-06-24 17:03 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Linux Kernel Network Developers

Hello, Stephen. Thanks for feedback.

I'm stuck in the error description. I can use just
  invarg("mutually exclusive options", *argv)

But from the user's point of view it isn't completely clear. I think it's
better to return a little bit more detailed error message. Something like this:
  if (filter_parent)
      invarg("parent is already specified", *argv);
  else if (filter_handle)
      invarg("handle is already specified", *argv);

Also I have declined the idea about adding the get command. Think the show
command is enough, and usage of the one more alias is unnecessary.

What do you think about this?

-- 
Anton Danilov.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH iproute2 v3] tc: improve the qdisc show command
  2020-06-18 22:17 ` Stephen Hemminger
  2020-06-24 17:03   ` Anton Danilov
@ 2020-07-03 15:39   ` Anton Danilov
  2020-07-06 18:01     ` Stephen Hemminger
  1 sibling, 1 reply; 5+ messages in thread
From: Anton Danilov @ 2020-07-03 15:39 UTC (permalink / raw)
  To: netdev; +Cc: Stephen Hemminger, Anton Danilov

Before can be possible show only all qeueue disciplines on an interface.
There wasn't a way to get the qdisc info by handle or parent, only full
dump of the disciplines with a following grep/sed usage.

Now new and old options work as expected to filter a qdisc by handle or
parent.

Full syntax of the qdisc show command:

tc qdisc { show | list } [ dev STRING ] [ QDISC_ID ] [ invisible ]
  QDISC_ID := { root | ingress | handle QHANDLE | parent CLASSID }

This change doesn't require any changes in the kernel.

Signed-off-by: Anton Danilov <littlesmilingcloud@gmail.com>
---
v2:
- Fix the coding style
v3:
- Make the parameters checking more simple
---
 man/man8/tc.8 |  8 +++--
 tc/tc_qdisc.c | 91 ++++++++++++++++++++++++++++++++-------------------
 2 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/man/man8/tc.8 b/man/man8/tc.8
index 235216b6..305bc569 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -77,9 +77,13 @@ tc \- show / manipulate traffic control settings
 .B tc
 .RI "[ " OPTIONS " ]"
 .RI "[ " FORMAT " ]"
-.B qdisc show [ dev
+.B qdisc { show | list } [ dev
 \fIDEV\fR
-.B ]
+.B ] [ root | ingress | handle
+\fIQHANDLE\fR
+.B | parent
+\fICLASSID\fR
+.B ] [ invisible ]
 .P
 .B tc
 .RI "[ " OPTIONS " ]"
diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index 181fe2f0..8eb08c34 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -35,11 +35,12 @@ static int usage(void)
 		"       [ ingress_block BLOCK_INDEX ] [ egress_block BLOCK_INDEX ]\n"
 		"       [ [ QDISC_KIND ] [ help | OPTIONS ] ]\n"
 		"\n"
-		"       tc qdisc show [ dev STRING ] [ ingress | clsact ] [ invisible ]\n"
+		"       tc qdisc { show | list } [ dev STRING ] [ QDISC_ID ] [ invisible ]\n"
 		"Where:\n"
 		"QDISC_KIND := { [p|b]fifo | tbf | prio | cbq | red | etc. }\n"
 		"OPTIONS := ... try tc qdisc add <desired QDISC_KIND> help\n"
-		"STAB_OPTIONS := ... try tc qdisc add stab help\n");
+		"STAB_OPTIONS := ... try tc qdisc add stab help\n"
+		"QDISC_ID := { root | ingress | handle QHANDLE | parent CLASSID }\n");
 	return -1;
 }
 
@@ -212,6 +213,8 @@ static int tc_qdisc_modify(int cmd, unsigned int flags, int argc, char **argv)
 }
 
 static int filter_ifindex;
+static __u32 filter_parent;
+static __u32 filter_handle;
 
 int print_qdisc(struct nlmsghdr *n, void *arg)
 {
@@ -235,6 +238,12 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
 	if (filter_ifindex && filter_ifindex != t->tcm_ifindex)
 		return 0;
 
+	if (filter_handle && filter_handle != t->tcm_handle)
+		return 0;
+
+	if (filter_parent && filter_parent != t->tcm_parent)
+		return 0;
+
 	parse_rtattr_flags(tb, TCA_MAX, TCA_RTA(t), len, NLA_F_NESTED);
 
 	if (tb[TCA_KIND] == NULL) {
@@ -344,21 +353,55 @@ int print_qdisc(struct nlmsghdr *n, void *arg)
 
 static int tc_qdisc_list(int argc, char **argv)
 {
-	struct tcmsg t = { .tcm_family = AF_UNSPEC };
+	struct {
+		struct nlmsghdr n;
+		struct tcmsg t;
+		char buf[256];
+	} req = {
+		.n.nlmsg_type = RTM_GETQDISC,
+		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
+		.t.tcm_family = AF_UNSPEC,
+	};
+
 	char d[IFNAMSIZ] = {};
 	bool dump_invisible = false;
+	__u32 handle;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
 			strncpy(d, *argv, sizeof(d)-1);
+		} else if (strcmp(*argv, "root") == 0) {
+			if (filter_parent)
+				invarg("parent is already specified", *argv);
+			else if (filter_handle)
+				invarg("handle is already specified", *argv);
+			filter_parent = TC_H_ROOT;
 		} else if (strcmp(*argv, "ingress") == 0 ||
-			   strcmp(*argv, "clsact") == 0) {
-			if (t.tcm_parent) {
-				fprintf(stderr, "Duplicate parent ID\n");
-				usage();
-			}
-			t.tcm_parent = TC_H_INGRESS;
+				strcmp(*argv, "clsact") == 0) {
+			if (filter_parent)
+				invarg("parent is already specified", *argv);
+			else if (filter_handle)
+				invarg("handle is already specified", *argv);
+			filter_parent = TC_H_INGRESS;
+		} else if (matches(*argv, "parent") == 0) {
+			if (filter_parent)
+				invarg("parent is already specified", *argv);
+			else if (filter_handle)
+				invarg("handle is already specified", *argv);
+			NEXT_ARG();
+			if (get_tc_classid(&handle, *argv))
+				invarg("invalid parent ID", *argv);
+			filter_parent = handle;
+		} else if (matches(*argv, "handle") == 0) {
+			if (filter_parent)
+				invarg("parent is already specified", *argv);
+			else if (filter_handle)
+				invarg("handle is already specified", *argv);
+			NEXT_ARG();
+			if (get_qdisc_handle(&handle, *argv))
+				invarg("invalid handle ID", *argv);
+			filter_handle = handle;
 		} else if (matches(*argv, "help") == 0) {
 			usage();
 		} else if (strcmp(*argv, "invisible") == 0) {
@@ -374,32 +417,18 @@ static int tc_qdisc_list(int argc, char **argv)
 	ll_init_map(&rth);
 
 	if (d[0]) {
-		t.tcm_ifindex = ll_name_to_index(d);
-		if (!t.tcm_ifindex)
+		req.t.tcm_ifindex = ll_name_to_index(d);
+		if (!req.t.tcm_ifindex)
 			return -nodev(d);
-		filter_ifindex = t.tcm_ifindex;
+		filter_ifindex = req.t.tcm_ifindex;
 	}
 
 	if (dump_invisible) {
-		struct {
-			struct nlmsghdr n;
-			struct tcmsg t;
-			char buf[256];
-		} req = {
-			.n.nlmsg_type = RTM_GETQDISC,
-			.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
-		};
-
-		req.t.tcm_family = AF_UNSPEC;
-
 		addattr(&req.n, 256, TCA_DUMP_INVISIBLE);
-		if (rtnl_dump_request_n(&rth, &req.n) < 0) {
-			perror("Cannot send dump request");
-			return 1;
-		}
+	}
 
-	} else if (rtnl_dump_request(&rth, RTM_GETQDISC, &t, sizeof(t)) < 0) {
-		perror("Cannot send dump request");
+	if (rtnl_dump_request_n(&rth, &req.n) < 0) {
+		perror("Cannot send request");
 		return 1;
 	}
 
@@ -427,10 +456,6 @@ int do_qdisc(int argc, char **argv)
 		return tc_qdisc_modify(RTM_NEWQDISC, NLM_F_REPLACE, argc-1, argv+1);
 	if (matches(*argv, "delete") == 0)
 		return tc_qdisc_modify(RTM_DELQDISC, 0,  argc-1, argv+1);
-#if 0
-	if (matches(*argv, "get") == 0)
-		return tc_qdisc_get(RTM_GETQDISC, 0,  argc-1, argv+1);
-#endif
 	if (matches(*argv, "list") == 0 || matches(*argv, "show") == 0
 	    || matches(*argv, "lst") == 0)
 		return tc_qdisc_list(argc-1, argv+1);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH iproute2 v3] tc: improve the qdisc show command
  2020-07-03 15:39   ` [PATCH iproute2 v3] tc: improve the qdisc show command Anton Danilov
@ 2020-07-06 18:01     ` Stephen Hemminger
  0 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2020-07-06 18:01 UTC (permalink / raw)
  To: Anton Danilov; +Cc: netdev

On Fri,  3 Jul 2020 18:39:22 +0300
Anton Danilov <littlesmilingcloud@gmail.com> wrote:

> Before can be possible show only all qeueue disciplines on an interface.
> There wasn't a way to get the qdisc info by handle or parent, only full
> dump of the disciplines with a following grep/sed usage.
> 
> Now new and old options work as expected to filter a qdisc by handle or
> parent.
> 
> Full syntax of the qdisc show command:
> 
> tc qdisc { show | list } [ dev STRING ] [ QDISC_ID ] [ invisible ]
>   QDISC_ID := { root | ingress | handle QHANDLE | parent CLASSID }
> 
> This change doesn't require any changes in the kernel.
> 
> Signed-off-by: Anton Danilov <littlesmilingcloud@gmail.com>
> ---
> v2:
> - Fix the coding style
> v3:
> - Make the parameters checking more simple

Applied, thanks

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-07-06 18:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16  6:39 [PATCH v2] tc: qdisc: filter qdisc's by parent/handle specification Anton Danilov
2020-06-18 22:17 ` Stephen Hemminger
2020-06-24 17:03   ` Anton Danilov
2020-07-03 15:39   ` [PATCH iproute2 v3] tc: improve the qdisc show command Anton Danilov
2020-07-06 18:01     ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).