[iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[Amritha Nambiar]

Added support for filtering based on port ranges.
UAPI changes have been accepted into net-next.

Example:
1. Match on a port range:
-------------------------
$ tc filter add dev enp4s0 protocol ip parent ffff:\
  prio 1 flower ip_proto tcp dst_port range 20-30 skip_hw\
  action drop

$ tc -s filter show dev enp4s0 parent ffff:
filter protocol ip pref 1 flower chain 0
filter protocol ip pref 1 flower chain 0 handle 0x1
  eth_type ipv4
  ip_proto tcp
  dst_port range 20-30
  skip_hw
  not_in_hw
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1 installed 85 sec used 3 sec
        Action statistics:
        Sent 460 bytes 10 pkt (dropped 10, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

2. Match on IP address and port range:
--------------------------------------
$ tc filter add dev enp4s0 protocol ip parent ffff:\
  prio 1 flower dst_ip 192.168.1.1 ip_proto tcp dst_port range 100-200\
  skip_hw action drop

$ tc -s filter show dev enp4s0 parent ffff:
filter protocol ip pref 1 flower chain 0 handle 0x2
  eth_type ipv4
  ip_proto tcp
  dst_ip 192.168.1.1
  dst_port range 100-200
  skip_hw
  not_in_hw
        action order 1: gact action drop
         random type none pass val 0
         index 2 ref 1 bind 1 installed 58 sec used 2 sec
        Action statistics:
        Sent 920 bytes 20 pkt (dropped 20, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

v4:
Added man updates explaining filtering based on port ranges.
Removed 'range' keyword.

v3:
Modified flower_port_range_attr_type calls.

v2:
Addressed Jiri's comment to sync output format with input

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
 man/man8/tc-flower.8 |   13 +++--
 tc/f_flower.c        |  136 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 134 insertions(+), 15 deletions(-)

diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index 8be8882..adff41e 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -56,8 +56,9 @@ flower \- flow based traffic control filter
 .IR MASKED_IP_TTL " | { "
 .BR dst_ip " | " src_ip " } "
 .IR PREFIX " | { "
-.BR dst_port " | " src_port " } "
-.IR port_number " } | "
+.BR dst_port " | " src_port " } { "
+.IR port_number " | "
+.IR min_port_number-max_port_number " } | "
 .B tcp_flags
 .IR MASKED_TCP_FLAGS " | "
 .B type
@@ -220,10 +221,12 @@ must be a valid IPv4 or IPv6 address, depending on the \fBprotocol\fR
 option to tc filter, optionally followed by a slash and the prefix length.
 If the prefix is missing, \fBtc\fR assumes a full-length host match.
 .TP
-.BI dst_port " NUMBER"
+.IR \fBdst_port " { "  NUMBER " | " " MIN_VALUE-MAX_VALUE "  }
 .TQ
-.BI src_port " NUMBER"
-Match on layer 4 protocol source or destination port number. Only available for
+.IR \fBsrc_port " { "  NUMBER " | " " MIN_VALUE-MAX_VALUE "  }
+Match on layer 4 protocol source or destination port number. Alternatively, the
+mininum and maximum values can be specified to match on a range of layer 4
+protocol source or destination port numbers. Only available for
 .BR ip_proto " values " udp ", " tcp  " and " sctp
 which have to be specified in beforehand.
 .TP
diff --git a/tc/f_flower.c b/tc/f_flower.c
index 65fca04..722647d 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -494,6 +494,68 @@ static int flower_parse_port(char *str, __u8 ip_proto,
 	return 0;
 }
 
+static int flower_port_range_attr_type(__u8 ip_proto, enum flower_endpoint type,
+				       __be16 *min_port_type,
+				       __be16 *max_port_type)
+{
+	if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP ||
+	    ip_proto == IPPROTO_SCTP) {
+		if (type == FLOWER_ENDPOINT_SRC) {
+			*min_port_type = TCA_FLOWER_KEY_PORT_SRC_MIN;
+			*max_port_type = TCA_FLOWER_KEY_PORT_SRC_MAX;
+		} else {
+			*min_port_type = TCA_FLOWER_KEY_PORT_DST_MIN;
+			*max_port_type = TCA_FLOWER_KEY_PORT_DST_MAX;
+		}
+	} else {
+		return -1;
+	}
+
+	return 0;
+}
+
+static int flower_parse_port_range(__be16 *min, __be16 *max, __u8 ip_proto,
+				   enum flower_endpoint endpoint,
+				   struct nlmsghdr *n)
+{
+	__be16 min_port_type, max_port_type;
+
+	if (htons(*max) <= htons(*min)) {
+		fprintf(stderr, "max value should be greater than min value\n");
+		return -1;
+	}
+
+	if (flower_port_range_attr_type(ip_proto, endpoint, &min_port_type,
+					&max_port_type))
+		return -1;
+
+	addattr16(n, MAX_MSG, min_port_type, *min);
+	addattr16(n, MAX_MSG, max_port_type, *max);
+
+	return 0;
+}
+
+static int get_range(__be16 *min, __be16 *max, char *argv)
+{
+	char *r;
+
+	r = strchr(argv, '-');
+	if (r) {
+		*r = '\0';
+		if (get_be16(min, argv, 10)) {
+			fprintf(stderr, "invalid min range\n");
+			return -1;
+		}
+		if (get_be16(max, r + 1, 10)) {
+			fprintf(stderr, "invalid max range\n");
+			return -1;
+		}
+	} else {
+		return -1;
+	}
+	return 0;
+}
+
 #define TCP_FLAGS_MAX_MASK 0xfff
 
 static int flower_parse_tcp_flags(char *str, int flags_type, int mask_type,
@@ -1061,20 +1123,47 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 				return -1;
 			}
 		} else if (matches(*argv, "dst_port") == 0) {
+			__be16 min, max;
+
 			NEXT_ARG();
-			ret = flower_parse_port(*argv, ip_proto,
-						FLOWER_ENDPOINT_DST, n);
-			if (ret < 0) {
-				fprintf(stderr, "Illegal \"dst_port\"\n");
-				return -1;
+
+			if (!get_range(&min, &max, *argv)) {
+				ret = flower_parse_port_range(&min, &max,
+							      ip_proto,
+							      FLOWER_ENDPOINT_DST,
+							      n);
+				if (ret < 0) {
+					fprintf(stderr, "Illegal \"dst_port range\"\n");
+					return -1;
+				}
+			} else {
+				ret = flower_parse_port(*argv, ip_proto,
+							FLOWER_ENDPOINT_DST, n);
+				if (ret < 0) {
+					fprintf(stderr, "Illegal \"dst_port\"\n");
+					return -1;
+				}
 			}
 		} else if (matches(*argv, "src_port") == 0) {
+			__be16 min, max;
+
 			NEXT_ARG();
-			ret = flower_parse_port(*argv, ip_proto,
-						FLOWER_ENDPOINT_SRC, n);
-			if (ret < 0) {
-				fprintf(stderr, "Illegal \"src_port\"\n");
-				return -1;
+			if (!get_range(&min, &max, *argv)) {
+				ret = flower_parse_port_range(&min, &max,
+							      ip_proto,
+							      FLOWER_ENDPOINT_SRC,
+							      n);
+				if (ret < 0) {
+					fprintf(stderr, "Illegal \"src_port range\"\n");
+					return -1;
+				}
+			} else {
+				ret = flower_parse_port(*argv, ip_proto,
+							FLOWER_ENDPOINT_SRC, n);
+				if (ret < 0) {
+					fprintf(stderr, "Illegal \"src_port\"\n");
+					return -1;
+				}
 			}
 		} else if (matches(*argv, "tcp_flags") == 0) {
 			NEXT_ARG();
@@ -1490,6 +1579,22 @@ static void flower_print_port(char *name, struct rtattr *attr)
 	print_hu(PRINT_ANY, name, namefrm, rta_getattr_be16(attr));
 }
 
+static void flower_print_port_range(char *name, struct rtattr *min_attr,
+				    struct rtattr *max_attr)
+{
+	SPRINT_BUF(namefrm);
+	SPRINT_BUF(out);
+	size_t done;
+
+	if (!min_attr || !max_attr)
+		return;
+
+	done = sprintf(out, "%u", rta_getattr_be16(min_attr));
+	sprintf(out + done, "-%u", rta_getattr_be16(max_attr));
+	sprintf(namefrm, "\n  %s %%s", name);
+	print_string(PRINT_ANY, name, namefrm, out);
+}
+
 static void flower_print_tcp_flags(const char *name, struct rtattr *flags_attr,
 				   struct rtattr *mask_attr)
 {
@@ -1678,6 +1783,7 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
 			    struct rtattr *opt, __u32 handle)
 {
 	struct rtattr *tb[TCA_FLOWER_MAX + 1];
+	__be16 min_port_type, max_port_type;
 	int nl_type, nl_mask_type;
 	__be16 eth_type = 0;
 	__u8 ip_proto = 0xff;
@@ -1796,6 +1902,16 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
 	if (nl_type >= 0)
 		flower_print_port("src_port", tb[nl_type]);
 
+	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_DST,
+					 &min_port_type, &max_port_type))
+		flower_print_port_range("dst_port range",
+					tb[min_port_type], tb[max_port_type]);
+
+	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_SRC,
+					 &min_port_type, &max_port_type))
+		flower_print_port_range("src_port range",
+					tb[min_port_type], tb[max_port_type]);
+
 	flower_print_tcp_flags("tcp_flags", tb[TCA_FLOWER_KEY_TCP_FLAGS],
 			       tb[TCA_FLOWER_KEY_TCP_FLAGS_MASK]);
 

Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[David Ahern]

On 11/20/18 11:17 PM, Amritha Nambiar wrote:
> diff --git a/tc/f_flower.c b/tc/f_flower.c
> index 65fca04..722647d 100644
> --- a/tc/f_flower.c
> +++ b/tc/f_flower.c
> @@ -494,6 +494,68 @@ static int flower_parse_port(char *str, __u8 ip_proto,
>  	return 0;
>  }
>  
> +static int flower_port_range_attr_type(__u8 ip_proto, enum flower_endpoint type,
> +				       __be16 *min_port_type,
> +				       __be16 *max_port_type)
> +{
> +	if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP ||
> +	    ip_proto == IPPROTO_SCTP) {
> +		if (type == FLOWER_ENDPOINT_SRC) {
> +			*min_port_type = TCA_FLOWER_KEY_PORT_SRC_MIN;
> +			*max_port_type = TCA_FLOWER_KEY_PORT_SRC_MAX;
> +		} else {
> +			*min_port_type = TCA_FLOWER_KEY_PORT_DST_MIN;
> +			*max_port_type = TCA_FLOWER_KEY_PORT_DST_MAX;
> +		}
> +	} else {
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int flower_parse_port_range(__be16 *min, __be16 *max, __u8 ip_proto,

why not just min and max directly since they are not set here but only
referenced by value. Also, you do not parse anything in this function so
the helper is misnamed.

But I think this can be done simpler using what was done in ip/iprule.c ...


> +				   enum flower_endpoint endpoint,
> +				   struct nlmsghdr *n)
> +{
> +	__be16 min_port_type, max_port_type;
> +
> +	if (htons(*max) <= htons(*min)) {
> +		fprintf(stderr, "max value should be greater than min value\n");
> +		return -1;
> +	}
> +
> +	if (flower_port_range_attr_type(ip_proto, endpoint, &min_port_type,
> +					&max_port_type))
> +		return -1;
> +
> +	addattr16(n, MAX_MSG, min_port_type, *min);
> +	addattr16(n, MAX_MSG, max_port_type, *max);
> +
> +	return 0;
> +}
> +
> +static int get_range(__be16 *min, __be16 *max, char *argv)
> +{
> +	char *r;
> +
> +	r = strchr(argv, '-');
> +	if (r) {
> +		*r = '\0';
> +		if (get_be16(min, argv, 10)) {
> +			fprintf(stderr, "invalid min range\n");
> +			return -1;
> +		}
> +		if (get_be16(max, r + 1, 10)) {
> +			fprintf(stderr, "invalid max range\n");
> +			return -1;
> +		}
> +	} else {
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  #define TCP_FLAGS_MAX_MASK 0xfff
>  
>  static int flower_parse_tcp_flags(char *str, int flags_type, int mask_type,
> @@ -1061,20 +1123,47 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
>  				return -1;
>  			}
>  		} else if (matches(*argv, "dst_port") == 0) {
> +			__be16 min, max;
> +
>  			NEXT_ARG();
> -			ret = flower_parse_port(*argv, ip_proto,
> -						FLOWER_ENDPOINT_DST, n);
> -			if (ret < 0) {
> -				fprintf(stderr, "Illegal \"dst_port\"\n");
> -				return -1;
> +
> +			if (!get_range(&min, &max, *argv)) {
> +				ret = flower_parse_port_range(&min, &max,
> +							      ip_proto,
> +							      FLOWER_ENDPOINT_DST,
> +							      n);
> +				if (ret < 0) {
> +					fprintf(stderr, "Illegal \"dst_port range\"\n");
> +					return -1;
> +				}
> +			} else {
> +				ret = flower_parse_port(*argv, ip_proto,
> +							FLOWER_ENDPOINT_DST, n);
> +				if (ret < 0) {
> +					fprintf(stderr, "Illegal \"dst_port\"\n");
> +					return -1;
> +				}

Take a look at ip/iprule.c, line 921:
		} else if (strcmp(*argv, "sport") == 0) {
			...
		}

Using sscanf and handling the ret to be 1 or 2 should simplify the above.

>  			}
>  		} else if (matches(*argv, "src_port") == 0) {
> +			__be16 min, max;
> +
>  			NEXT_ARG();
> -			ret = flower_parse_port(*argv, ip_proto,
> -						FLOWER_ENDPOINT_SRC, n);
> -			if (ret < 0) {
> -				fprintf(stderr, "Illegal \"src_port\"\n");
> -				return -1;
> +			if (!get_range(&min, &max, *argv)) {
> +				ret = flower_parse_port_range(&min, &max,
> +							      ip_proto,
> +							      FLOWER_ENDPOINT_SRC,
> +							      n);
> +				if (ret < 0) {
> +					fprintf(stderr, "Illegal \"src_port range\"\n");
> +					return -1;
> +				}
> +			} else {
> +				ret = flower_parse_port(*argv, ip_proto,
> +							FLOWER_ENDPOINT_SRC, n);
> +				if (ret < 0) {
> +					fprintf(stderr, "Illegal \"src_port\"\n");
> +					return -1;
> +				}
>  			}
>  		} else if (matches(*argv, "tcp_flags") == 0) {
>  			NEXT_ARG();
> @@ -1490,6 +1579,22 @@ static void flower_print_port(char *name, struct rtattr *attr)
>  	print_hu(PRINT_ANY, name, namefrm, rta_getattr_be16(attr));
>  }
>  
> +static void flower_print_port_range(char *name, struct rtattr *min_attr,
> +				    struct rtattr *max_attr)
> +{
> +	SPRINT_BUF(namefrm);
> +	SPRINT_BUF(out);
> +	size_t done;
> +
> +	if (!min_attr || !max_attr)
> +		return;
> +
> +	done = sprintf(out, "%u", rta_getattr_be16(min_attr));
> +	sprintf(out + done, "-%u", rta_getattr_be16(max_attr));
> +	sprintf(namefrm, "\n  %s %%s", name);
> +	print_string(PRINT_ANY, name, namefrm, out);
> +}
> +
>  static void flower_print_tcp_flags(const char *name, struct rtattr *flags_attr,
>  				   struct rtattr *mask_attr)
>  {
> @@ -1678,6 +1783,7 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
>  			    struct rtattr *opt, __u32 handle)
>  {
>  	struct rtattr *tb[TCA_FLOWER_MAX + 1];
> +	__be16 min_port_type, max_port_type;
>  	int nl_type, nl_mask_type;
>  	__be16 eth_type = 0;
>  	__u8 ip_proto = 0xff;
> @@ -1796,6 +1902,16 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
>  	if (nl_type >= 0)
>  		flower_print_port("src_port", tb[nl_type]);
>  
> +	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_DST,
> +					 &min_port_type, &max_port_type))
> +		flower_print_port_range("dst_port range",

I am no json expert, but I do not recall any other place where a space
is used in the name field for json output.

Can tc flower use something similar to ip ru with single port or port
range handled like this?

    },{
        "priority": 32764,
        "src": "172.16.1.0",
        "srclen": 24,
        "ipproto": "tcp",
        "sport": 1100,
        "table": "main"
    },{
        "priority": 32765,
        "src": "172.16.1.0",
        "srclen": 24,
        "ipproto": "tcp",
        "sport_start": 1000,
        "sport_end": 1010,
        "table": "main"
    },{


> +					tb[min_port_type], tb[max_port_type]);
> +
> +	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_SRC,
> +					 &min_port_type, &max_port_type))
> +		flower_print_port_range("src_port range",
> +					tb[min_port_type], tb[max_port_type]);
> +
>  	flower_print_tcp_flags("tcp_flags", tb[TCA_FLOWER_KEY_TCP_FLAGS],
>  			       tb[TCA_FLOWER_KEY_TCP_FLAGS_MASK]);
>  
> 

Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[Nambiar, Amritha]

On 11/21/2018 1:42 PM, David Ahern wrote:
> On 11/20/18 11:17 PM, Amritha Nambiar wrote:
>> diff --git a/tc/f_flower.c b/tc/f_flower.c
>> index 65fca04..722647d 100644
>> --- a/tc/f_flower.c
>> +++ b/tc/f_flower.c
>> @@ -494,6 +494,68 @@ static int flower_parse_port(char *str, __u8 ip_proto,
>>  	return 0;
>>  }
>>  
>> +static int flower_port_range_attr_type(__u8 ip_proto, enum flower_endpoint type,
>> +				       __be16 *min_port_type,
>> +				       __be16 *max_port_type)
>> +{
>> +	if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP ||
>> +	    ip_proto == IPPROTO_SCTP) {
>> +		if (type == FLOWER_ENDPOINT_SRC) {
>> +			*min_port_type = TCA_FLOWER_KEY_PORT_SRC_MIN;
>> +			*max_port_type = TCA_FLOWER_KEY_PORT_SRC_MAX;
>> +		} else {
>> +			*min_port_type = TCA_FLOWER_KEY_PORT_DST_MIN;
>> +			*max_port_type = TCA_FLOWER_KEY_PORT_DST_MAX;
>> +		}
>> +	} else {
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int flower_parse_port_range(__be16 *min, __be16 *max, __u8 ip_proto,
> 
> why not just min and max directly since they are not set here but only
> referenced by value. Also, you do not parse anything in this function so
> the helper is misnamed.
> 
> But I think this can be done simpler using what was done in ip/iprule.c ...
> 

Okay, will modify this.

> 
>> +				   enum flower_endpoint endpoint,
>> +				   struct nlmsghdr *n)
>> +{
>> +	__be16 min_port_type, max_port_type;
>> +
>> +	if (htons(*max) <= htons(*min)) {
>> +		fprintf(stderr, "max value should be greater than min value\n");
>> +		return -1;
>> +	}
>> +
>> +	if (flower_port_range_attr_type(ip_proto, endpoint, &min_port_type,
>> +					&max_port_type))
>> +		return -1;
>> +
>> +	addattr16(n, MAX_MSG, min_port_type, *min);
>> +	addattr16(n, MAX_MSG, max_port_type, *max);
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_range(__be16 *min, __be16 *max, char *argv)
>> +{
>> +	char *r;
>> +
>> +	r = strchr(argv, '-');
>> +	if (r) {
>> +		*r = '\0';
>> +		if (get_be16(min, argv, 10)) {
>> +			fprintf(stderr, "invalid min range\n");
>> +			return -1;
>> +		}
>> +		if (get_be16(max, r + 1, 10)) {
>> +			fprintf(stderr, "invalid max range\n");
>> +			return -1;
>> +		}
>> +	} else {
>> +		return -1;
>> +	}
>> +	return 0;
>> +}
>> +
>>  #define TCP_FLAGS_MAX_MASK 0xfff
>>  
>>  static int flower_parse_tcp_flags(char *str, int flags_type, int mask_type,
>> @@ -1061,20 +1123,47 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
>>  				return -1;
>>  			}
>>  		} else if (matches(*argv, "dst_port") == 0) {
>> +			__be16 min, max;
>> +
>>  			NEXT_ARG();
>> -			ret = flower_parse_port(*argv, ip_proto,
>> -						FLOWER_ENDPOINT_DST, n);
>> -			if (ret < 0) {
>> -				fprintf(stderr, "Illegal \"dst_port\"\n");
>> -				return -1;
>> +
>> +			if (!get_range(&min, &max, *argv)) {
>> +				ret = flower_parse_port_range(&min, &max,
>> +							      ip_proto,
>> +							      FLOWER_ENDPOINT_DST,
>> +							      n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"dst_port range\"\n");
>> +					return -1;
>> +				}
>> +			} else {
>> +				ret = flower_parse_port(*argv, ip_proto,
>> +							FLOWER_ENDPOINT_DST, n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"dst_port\"\n");
>> +					return -1;
>> +				}
> 
> Take a look at ip/iprule.c, line 921:
> 		} else if (strcmp(*argv, "sport") == 0) {
> 			...
> 		}
> 
> Using sscanf and handling the ret to be 1 or 2 should simplify the above.
> 

Okay, will simplify using sscanf.

>>  			}
>>  		} else if (matches(*argv, "src_port") == 0) {
>> +			__be16 min, max;
>> +
>>  			NEXT_ARG();
>> -			ret = flower_parse_port(*argv, ip_proto,
>> -						FLOWER_ENDPOINT_SRC, n);
>> -			if (ret < 0) {
>> -				fprintf(stderr, "Illegal \"src_port\"\n");
>> -				return -1;
>> +			if (!get_range(&min, &max, *argv)) {
>> +				ret = flower_parse_port_range(&min, &max,
>> +							      ip_proto,
>> +							      FLOWER_ENDPOINT_SRC,
>> +							      n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"src_port range\"\n");
>> +					return -1;
>> +				}
>> +			} else {
>> +				ret = flower_parse_port(*argv, ip_proto,
>> +							FLOWER_ENDPOINT_SRC, n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"src_port\"\n");
>> +					return -1;
>> +				}
>>  			}
>>  		} else if (matches(*argv, "tcp_flags") == 0) {
>>  			NEXT_ARG();
>> @@ -1490,6 +1579,22 @@ static void flower_print_port(char *name, struct rtattr *attr)
>>  	print_hu(PRINT_ANY, name, namefrm, rta_getattr_be16(attr));
>>  }
>>  
>> +static void flower_print_port_range(char *name, struct rtattr *min_attr,
>> +				    struct rtattr *max_attr)
>> +{
>> +	SPRINT_BUF(namefrm);
>> +	SPRINT_BUF(out);
>> +	size_t done;
>> +
>> +	if (!min_attr || !max_attr)
>> +		return;
>> +
>> +	done = sprintf(out, "%u", rta_getattr_be16(min_attr));
>> +	sprintf(out + done, "-%u", rta_getattr_be16(max_attr));
>> +	sprintf(namefrm, "\n  %s %%s", name);
>> +	print_string(PRINT_ANY, name, namefrm, out);
>> +}
>> +
>>  static void flower_print_tcp_flags(const char *name, struct rtattr *flags_attr,
>>  				   struct rtattr *mask_attr)
>>  {
>> @@ -1678,6 +1783,7 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
>>  			    struct rtattr *opt, __u32 handle)
>>  {
>>  	struct rtattr *tb[TCA_FLOWER_MAX + 1];
>> +	__be16 min_port_type, max_port_type;
>>  	int nl_type, nl_mask_type;
>>  	__be16 eth_type = 0;
>>  	__u8 ip_proto = 0xff;
>> @@ -1796,6 +1902,16 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
>>  	if (nl_type >= 0)
>>  		flower_print_port("src_port", tb[nl_type]);
>>  
>> +	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_DST,
>> +					 &min_port_type, &max_port_type))
>> +		flower_print_port_range("dst_port range",
> 
> I am no json expert, but I do not recall any other place where a space
> is used in the name field for json output.
> 

Will remove the space and range keyword.

> Can tc flower use something similar to ip ru with single port or port
> range handled like this?
> 
>     },{
>         "priority": 32764,
>         "src": "172.16.1.0",
>         "srclen": 24,
>         "ipproto": "tcp",
>         "sport": 1100,
>         "table": "main"
>     },{
>         "priority": 32765,
>         "src": "172.16.1.0",
>         "srclen": 24,
>         "ipproto": "tcp",
>         "sport_start": 1000,
>         "sport_end": 1010,
>         "table": "main"
>     },{
> 
> 

Does it have to be separate fields "sport_start" and "sport_end"?
Removing the space and 'range' keyword will make the output format
consistent with the input format and print as "sport <number>" for
single port and "sport <start>-<end>" for range.
Example:

... flower ip_proto tcp src_port 12 skip_hw action will print as:
  ip_proto tcp
  src_port 12
  skip_hw
  not_in_hw
        action

... flower ip_proto tcp src_port 100-200 skip_hw action :
  ip_proto tcp
  src_port 100-200
  skip_hw
  not_in_hw
        action
>> +					tb[min_port_type], tb[max_port_type]);
>> +
>> +	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_SRC,
>> +					 &min_port_type, &max_port_type))
>> +		flower_print_port_range("src_port range",
>> +					tb[min_port_type], tb[max_port_type]);
>> +
>>  	flower_print_tcp_flags("tcp_flags", tb[TCA_FLOWER_KEY_TCP_FLAGS],
>>  			       tb[TCA_FLOWER_KEY_TCP_FLAGS_MASK]);
>>  
>>
> 

Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[David Ahern]

On 11/26/18 5:23 PM, Nambiar, Amritha wrote:
>> Can tc flower use something similar to ip ru with single port or port
>> range handled like this?
>>
>>     },{
>>         "priority": 32764,
>>         "src": "172.16.1.0",
>>         "srclen": 24,
>>         "ipproto": "tcp",
>>         "sport": 1100,
>>         "table": "main"
>>     },{
>>         "priority": 32765,
>>         "src": "172.16.1.0",
>>         "srclen": 24,
>>         "ipproto": "tcp",
>>         "sport_start": 1000,
>>         "sport_end": 1010,
>>         "table": "main"
>>     },{
>>
>>
> 
> Does it have to be separate fields "sport_start" and "sport_end"?
> Removing the space and 'range' keyword will make the output format
> consistent with the input format and print as "sport <number>" for
> single port and "sport <start>-<end>" for range.
> Example:
> 
> ... flower ip_proto tcp src_port 12 skip_hw action will print as:
>   ip_proto tcp
>   src_port 12
>   skip_hw
>   not_in_hw
>         action
> 
> ... flower ip_proto tcp src_port 100-200 skip_hw action :
>   ip_proto tcp
>   src_port 100-200
>   skip_hw
>   not_in_hw
>         action

non-json output needs to match what the user gives on the command line.

My comment was about consistency with json output when possible. I am
not a json expert by any means. Other commands have a single key value
pair, so I suspect the json here needs to follow suit (ie., not
"src_port": "1000-1010" but separate start and end entries).

Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[Nambiar, Amritha]

On 11/26/2018 4:43 PM, David Ahern wrote:
> On 11/26/18 5:23 PM, Nambiar, Amritha wrote:
>>> Can tc flower use something similar to ip ru with single port or port
>>> range handled like this?
>>>
>>>     },{
>>>         "priority": 32764,
>>>         "src": "172.16.1.0",
>>>         "srclen": 24,
>>>         "ipproto": "tcp",
>>>         "sport": 1100,
>>>         "table": "main"
>>>     },{
>>>         "priority": 32765,
>>>         "src": "172.16.1.0",
>>>         "srclen": 24,
>>>         "ipproto": "tcp",
>>>         "sport_start": 1000,
>>>         "sport_end": 1010,
>>>         "table": "main"
>>>     },{
>>>
>>>
>>
>> Does it have to be separate fields "sport_start" and "sport_end"?
>> Removing the space and 'range' keyword will make the output format
>> consistent with the input format and print as "sport <number>" for
>> single port and "sport <start>-<end>" for range.
>> Example:
>>
>> ... flower ip_proto tcp src_port 12 skip_hw action will print as:
>>   ip_proto tcp
>>   src_port 12
>>   skip_hw
>>   not_in_hw
>>         action
>>
>> ... flower ip_proto tcp src_port 100-200 skip_hw action :
>>   ip_proto tcp
>>   src_port 100-200
>>   skip_hw
>>   not_in_hw
>>         action
> 
> non-json output needs to match what the user gives on the command line.
> 
> My comment was about consistency with json output when possible. I am
> not a json expert by any means. Other commands have a single key value
> pair, so I suspect the json here needs to follow suit (ie., not
> "src_port": "1000-1010" but separate start and end entries).
> 
I'm not quite familiar with json. Maybe, Jiri can give feedback here.

Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[Stephen Hemminger]

On Mon, 26 Nov 2018 17:56:10 -0800
"Nambiar, Amritha" <amritha.nambiar@intel.com> wrote:

> On 11/26/2018 4:43 PM, David Ahern wrote:
> > On 11/26/18 5:23 PM, Nambiar, Amritha wrote:  
> >>> Can tc flower use something similar to ip ru with single port or port
> >>> range handled like this?
> >>>
> >>>     },{
> >>>         "priority": 32764,
> >>>         "src": "172.16.1.0",
> >>>         "srclen": 24,
> >>>         "ipproto": "tcp",
> >>>         "sport": 1100,
> >>>         "table": "main"
> >>>     },{
> >>>         "priority": 32765,
> >>>         "src": "172.16.1.0",
> >>>         "srclen": 24,
> >>>         "ipproto": "tcp",
> >>>         "sport_start": 1000,
> >>>         "sport_end": 1010,
> >>>         "table": "main"
> >>>     },{
> >>>
> >>>  
> >>
> >> Does it have to be separate fields "sport_start" and "sport_end"?
> >> Removing the space and 'range' keyword will make the output format
> >> consistent with the input format and print as "sport <number>" for
> >> single port and "sport <start>-<end>" for range.
> >> Example:
> >>
> >> ... flower ip_proto tcp src_port 12 skip_hw action will print as:
> >>   ip_proto tcp
> >>   src_port 12
> >>   skip_hw
> >>   not_in_hw
> >>         action
> >>
> >> ... flower ip_proto tcp src_port 100-200 skip_hw action :
> >>   ip_proto tcp
> >>   src_port 100-200
> >>   skip_hw
> >>   not_in_hw
> >>         action  
> > 
> > non-json output needs to match what the user gives on the command line.
> > 
> > My comment was about consistency with json output when possible. I am
> > not a json expert by any means. Other commands have a single key value
> > pair, so I suspect the json here needs to follow suit (ie., not
> > "src_port": "1000-1010" but separate start and end entries).
> >   
> I'm not quite familiar with json. Maybe, Jiri can give feedback here.

JSON support strings and numeric and objects.
The more common JSON way of expressing this would be either as object for sport

{
       "priority": 32765,
       "src": "172.16.1.0",
        "srclen": 24,
       "ipproto": "tcp",
	"sport" : {
		"start" : 1000,
		"end" : 1010
	},
	"table: "main"
}

or as an array:
{
       "priority": 32765,
       "src": "172.16.1.0",
        "srclen": 24,
       "ipproto": "tcp",
	"sport" : [ 1000, 1010 ],
	"table: "main"
}

My point is don't build some semantic meaning directly into the tag part of the syntax.

Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges

[Nambiar, Amritha]

On 11/26/2018 8:02 PM, Stephen Hemminger wrote:
> On Mon, 26 Nov 2018 17:56:10 -0800
> "Nambiar, Amritha" <amritha.nambiar@intel.com> wrote:
> 
>> On 11/26/2018 4:43 PM, David Ahern wrote:
>>> On 11/26/18 5:23 PM, Nambiar, Amritha wrote:  
>>>>> Can tc flower use something similar to ip ru with single port or port
>>>>> range handled like this?
>>>>>
>>>>>     },{
>>>>>         "priority": 32764,
>>>>>         "src": "172.16.1.0",
>>>>>         "srclen": 24,
>>>>>         "ipproto": "tcp",
>>>>>         "sport": 1100,
>>>>>         "table": "main"
>>>>>     },{
>>>>>         "priority": 32765,
>>>>>         "src": "172.16.1.0",
>>>>>         "srclen": 24,
>>>>>         "ipproto": "tcp",
>>>>>         "sport_start": 1000,
>>>>>         "sport_end": 1010,
>>>>>         "table": "main"
>>>>>     },{
>>>>>
>>>>>  
>>>>
>>>> Does it have to be separate fields "sport_start" and "sport_end"?
>>>> Removing the space and 'range' keyword will make the output format
>>>> consistent with the input format and print as "sport <number>" for
>>>> single port and "sport <start>-<end>" for range.
>>>> Example:
>>>>
>>>> ... flower ip_proto tcp src_port 12 skip_hw action will print as:
>>>>   ip_proto tcp
>>>>   src_port 12
>>>>   skip_hw
>>>>   not_in_hw
>>>>         action
>>>>
>>>> ... flower ip_proto tcp src_port 100-200 skip_hw action :
>>>>   ip_proto tcp
>>>>   src_port 100-200
>>>>   skip_hw
>>>>   not_in_hw
>>>>         action  
>>>
>>> non-json output needs to match what the user gives on the command line.
>>>
>>> My comment was about consistency with json output when possible. I am
>>> not a json expert by any means. Other commands have a single key value
>>> pair, so I suspect the json here needs to follow suit (ie., not
>>> "src_port": "1000-1010" but separate start and end entries).
>>>   
>> I'm not quite familiar with json. Maybe, Jiri can give feedback here.
> 
> JSON support strings and numeric and objects.
> The more common JSON way of expressing this would be either as object for sport
> 
> {
>        "priority": 32765,
>        "src": "172.16.1.0",
>         "srclen": 24,
>        "ipproto": "tcp",
> 	"sport" : {
> 		"start" : 1000,
> 		"end" : 1010
> 	},
> 	"table: "main"
> }
> 
> or as an array:
> {
>        "priority": 32765,
>        "src": "172.16.1.0",
>         "srclen": 24,
>        "ipproto": "tcp",
> 	"sport" : [ 1000, 1010 ],
> 	"table: "main"
> }
> 
> My point is don't build some semantic meaning directly into the tag part of the syntax.
> 

Okay. Will fix this in v6. I'll keep the default (non-json) output
format similar to the input format, and for the json output, I'll use
the object for sport as in the first example above.