diff mbox series

[v2] tc: qdisc: filter qdisc's by parent/handle specification

Message ID 20200616063902.15605-1-littlesmilingcloud@gmail.com
State Changes Requested
Delegated to: stephen hemminger
Headers show
Series [v2] tc: qdisc: filter qdisc's by parent/handle specification | expand

Commit Message

Anton Danilov June 16, 2020, 6:39 a.m. UTC 
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(-)

Comments

Stephen Hemminger June 18, 2020, 10:17 p.m. UTC | #1 
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.
Anton Danilov June 24, 2020, 5:03 p.m. UTC | #2 
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?
diff mbox series

Patch

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