diff mbox

[iproute2,v2,1/1] tc simple action update and breakage

Message ID 1462719726-7598-1-git-send-email-jhs@emojatatu.com
State Accepted, archived
Delegated to: stephen hemminger
Headers show

Commit Message

Jamal Hadi Salim May 8, 2016, 3:02 p.m. UTC 
From: Jamal Hadi Salim <jhs@mojatatu.com>

Brings it closer to more serious actions (adding branching
and allowing for late binding)

Unfortunately this breaks old syntax of the simple action.
But because simple is a pedagogical example unlikely to be used
in production environments (i.e its role is to serve as an example
on how to write actions), then this is ok.

New syntax for simple has new keyword "sdata". Example usage is:

sudo tc actions add action simple sdata "foobar" index 1
or
tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
match ip dst 17.0.0.1/32 flowid 1:10 action simple sdata "foobar"

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 tc/m_simple.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 14 deletions(-)

Comments

Daniel Borkmann May 23, 2016, 10:39 p.m. UTC | #1 
On 05/08/2016 05:02 PM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Brings it closer to more serious actions (adding branching
> and allowing for late binding)
>
> Unfortunately this breaks old syntax of the simple action.
> But because simple is a pedagogical example unlikely to be used
> in production environments (i.e its role is to serve as an example
> on how to write actions), then this is ok.
>
> New syntax for simple has new keyword "sdata". Example usage is:
>
> sudo tc actions add action simple sdata "foobar" index 1
> or
> tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
> match ip dst 17.0.0.1/32 flowid 1:10 action simple sdata "foobar"
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
[...]

>   static void usage(void)
> @@ -99,56 +100,85 @@ parse_simple(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>   	struct tc_defact sel = {};
>   	int argc = *argc_p;
>   	char **argv = *argv_p;
> -	int ok = 0;
> +	int ok = 0, maybe_bind = 0;
>   	struct rtattr *tail;
>   	char *simpdata = NULL;
>
[...]

>   	if (argc) {
>   		if (matches(*argv, "index") == 0) {
>   			NEXT_ARG();
>   			if (get_u32(&sel.index, *argv, 10)) {
> -				fprintf(stderr, "simple: Illegal \"index\"\n");
> +				fprintf(stderr, "simple: Illegal \"index\"\n",
> +					*argv);
>   				return -1;
>   			}

Btw, this causes:

m_simple.c: In function ‘parse_simple’:
m_simple.c:154:6: warning: too many arguments for format [-Wformat-extra-args]
       *argv);
       ^
m_simple.c:103:14: warning: unused variable ‘maybe_bind’ [-Wunused-variable]
   int ok = 0, maybe_bind = 0;
               ^
Jamal Hadi Salim May 24, 2016, 12:59 a.m. UTC | #2 
On 16-05-23 06:39 PM, Daniel Borkmann wrote:

>
> m_simple.c: In function ‘parse_simple’:
> m_simple.c:154:6: warning: too many arguments for format
> [-Wformat-extra-args]
>        *argv);
>        ^
> m_simple.c:103:14: warning: unused variable ‘maybe_bind’
> [-Wunused-variable]
>    int ok = 0, maybe_bind = 0;
>                ^

Will send a fix. Thanks Daniel.

cheers,
jamal
diff mbox

Patch

diff --git a/tc/m_simple.c b/tc/m_simple.c
index e167cca..feba61b 100644
--- a/tc/m_simple.c
+++ b/tc/m_simple.c
@@ -81,9 +81,10 @@ 
 #endif
 static void explain(void)
 {
-	fprintf(stderr, "Usage: ... simple STRING\n"
-		"STRING being an arbitrary string\n"
-		"example: \"simple blah\"\n");
+	fprintf(stderr, "Usage:... simple [sdata STRING] [CONTROL] [index INDEX]\n");
+	fprintf(stderr, "\tSTRING being an arbitrary string\n"
+		"\tCONTROL := reclassify|pipe|drop|continue|ok\n"
+		"\tINDEX := optional index value used\n");
 }
 
 static void usage(void)
@@ -99,56 +100,85 @@  parse_simple(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
 	struct tc_defact sel = {};
 	int argc = *argc_p;
 	char **argv = *argv_p;
-	int ok = 0;
+	int ok = 0, maybe_bind = 0;
 	struct rtattr *tail;
 	char *simpdata = NULL;
 
-
 	while (argc > 0) {
 		if (matches(*argv, "simple") == 0) {
 			NEXT_ARG();
+		} else if (matches(*argv, "sdata") == 0) {
+			NEXT_ARG();
+			ok += 1;
 			simpdata = *argv;
-			ok = 1;
 			argc--;
 			argv++;
-			break;
 		} else if (matches(*argv, "help") == 0) {
 			usage();
 		} else {
 			break;
 		}
-
 	}
 
-	if (!ok) {
-		explain();
-		return -1;
+	if (argc) {
+		if (matches(*argv, "reclassify") == 0) {
+			sel.action = TC_ACT_RECLASSIFY;
+			argc--;
+			argv++;
+		} else if (matches(*argv, "pipe") == 0) {
+			sel.action = TC_ACT_PIPE;
+			argc--;
+			argv++;
+		} else if (matches(*argv, "drop") == 0 ||
+			   matches(*argv, "shot") == 0) {
+			sel.action = TC_ACT_SHOT;
+			argc--;
+			argv++;
+		} else if (matches(*argv, "continue") == 0) {
+			sel.action = TC_ACT_UNSPEC;
+			argc--;
+			argv++;
+		} else if (matches(*argv, "pass") == 0 ||
+			   matches(*argv, "ok") == 0) {
+			sel.action = TC_ACT_OK;
+			argc--;
+			argv++;
+		}
 	}
 
 	if (argc) {
 		if (matches(*argv, "index") == 0) {
 			NEXT_ARG();
 			if (get_u32(&sel.index, *argv, 10)) {
-				fprintf(stderr, "simple: Illegal \"index\"\n");
+				fprintf(stderr, "simple: Illegal \"index\"\n",
+					*argv);
 				return -1;
 			}
+			ok += 1;
 			argc--;
 			argv++;
 		}
 	}
 
-	if (strlen(simpdata) > (SIMP_MAX_DATA - 1)) {
+	if (!ok) {
+		explain();
+		return -1;
+	}
+
+	if (simpdata && (strlen(simpdata) > (SIMP_MAX_DATA - 1))) {
 		fprintf(stderr, "simple: Illegal string len %zu <%s>\n",
 			strlen(simpdata), simpdata);
 		return -1;
 	}
 
+
 	sel.action = TC_ACT_PIPE;
 
 	tail = NLMSG_TAIL(n);
 	addattr_l(n, MAX_MSG, tca_id, NULL, 0);
 	addattr_l(n, MAX_MSG, TCA_DEF_PARMS, &sel, sizeof(sel));
-	addattr_l(n, MAX_MSG, TCA_DEF_DATA, simpdata, SIMP_MAX_DATA);
+	if (simpdata)
+		addattr_l(n, MAX_MSG, TCA_DEF_DATA, simpdata, SIMP_MAX_DATA);
 	tail->rta_len = (char *)NLMSG_TAIL(n) - (char *)tail;
 
 	*argc_p = argc;