From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges Date: Wed, 21 Nov 2018 14:42:52 -0700 Message-ID: <4b11cf66-089d-1a13-fae9-d471de9064ea@gmail.com> References: <154278102460.66868.7581842092203300039.stgit@anamhost.jf.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: jakub.kicinski@netronome.com, sridhar.samudrala@intel.com, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us To: Amritha Nambiar , stephen@networkplumber.org, netdev@vger.kernel.org Return-path: Received: from mail-pl1-f194.google.com ([209.85.214.194]:39864 "EHLO mail-pl1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729727AbeKVITD (ORCPT ); Thu, 22 Nov 2018 03:19:03 -0500 Received: by mail-pl1-f194.google.com with SMTP id 101so996694pld.6 for ; Wed, 21 Nov 2018 13:42:55 -0800 (PST) In-Reply-To: <154278102460.66868.7581842092203300039.stgit@anamhost.jf.intel.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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]); > >