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