Re: [PATCH iproute2 V2 1/2] tc/cls_flower: Classify packet in ip tunnels

Wed, Nov 30, 2016 at 08:38:24AM CET, amirva@gmail.com wrote:

On Wed, Nov 30, 2016 at 9:17 AM Amir Vadai [off-list ref] wrote:

quoted

On Tue, Nov 29, 2016 at 07:26:58PM -0800, Stephen Hemminger wrote:

quoted

The overall design is fine, just a couple nits with the code.

quoted

diff --git a/tc/f_flower.c b/tc/f_flower.c
index 2d31d1aa832d..1cf0750b5b83 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c

quoted

+static int flower_parse_key_id(char *str, int type, struct nlmsghdr

*n)

quoted

str is not modified, therefore use: const char *str

ack

quoted

quoted

+{
+   int ret;
+   __be32 key_id;
+
+   ret = get_be32(&key_id, str, 10);
+   if (ret)
+           return -1;

Traditionally netlink attributes are in host order, why was flower
chosen to be different?

I don't know, maybe Jiri (cc'ed) can explain, but it is all over the
flower code.

Now the right Jiri (Pirko) is CC'ed

There is a bunch of helpers inside the cls_flower.c to put and set
values, they work with generic char arrays of len.

quoted

quoted

quoted

+
+   addattr32(n, MAX_MSG, type, key_id);
+
+   return 0;

Why lose the return value here?  Why not:

      ret = get_be32(&key_id, str, 10);
      if (!ret)
              addattr32(n, MAX_MSG, type, key_id);

The get_*() function can return only -1 or 0. But you are right, and it
is better the way you suggested.  Changing accordingly in V3.

quoted

quoted

+}
+
 static int flower_parse_opt(struct filter_util *qu, char *handle,
                        int argc, char **argv, struct nlmsghdr *n)
 {

@@ -339,6 +359,38 @@ static int flower_parse_opt(struct filter_util

*qu, char *handle,

quoted

quoted

                            fprintf(stderr, "Illegal \"src_port\"\n");
                            return -1;
                    }
+           } else if (matches(*argv, "enc_dst_ip") == 0) {
+                   NEXT_ARG();
+                   ret = flower_parse_ip_addr(*argv, 0,
+

TCA_FLOWER_KEY_ENC_IPV4_DST,

quoted

quoted

+

TCA_FLOWER_KEY_ENC_IPV4_DST_MASK,

quoted

quoted

+

TCA_FLOWER_KEY_ENC_IPV6_DST,

quoted

quoted

+

TCA_FLOWER_KEY_ENC_IPV6_DST_MASK,

quoted

quoted

+                                              n);
+                   if (ret < 0) {
+                           fprintf(stderr, "Illegal

\"enc_dst_ip\"\n");

quoted

quoted

+                           return -1;
+                   }
+           } else if (matches(*argv, "enc_src_ip") == 0) {
+                   NEXT_ARG();
+                   ret = flower_parse_ip_addr(*argv, 0,
+

TCA_FLOWER_KEY_ENC_IPV4_SRC,

quoted

quoted

+

TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK,

quoted

quoted

+

TCA_FLOWER_KEY_ENC_IPV6_SRC,

quoted

quoted

+

TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK,

quoted

quoted

+                                              n);
+                   if (ret < 0) {
+                           fprintf(stderr, "Illegal

\"enc_src_ip\"\n");

quoted

quoted

+                           return -1;
+                   }
+           } else if (matches(*argv, "enc_key_id") == 0) {
+                   NEXT_ARG();
+                   ret = flower_parse_key_id(*argv,
+

 TCA_FLOWER_KEY_ENC_KEY_ID, n);

quoted

quoted

+                   if (ret < 0) {
+                           fprintf(stderr, "Illegal

\"enc_key_id\"\n");

quoted

quoted

+                           return -1;
+                   }
            } else if (matches(*argv, "action") == 0) {
                    NEXT_ARG();
                    ret = parse_action(&argc, &argv, TCA_FLOWER_ACT,

n);

quoted

quoted

@@ -509,6 +561,14 @@ static void flower_print_port(FILE *f, char

*name, __u8 ip_proto,

quoted

quoted

    fprintf(f, "\n  %s %d", name, ntohs(rta_getattr_u16(attr)));
 }

+static void flower_print_key_id(FILE *f, char *name,
+                           struct rtattr *attr)

const char *name?

ack

quoted

quoted

+{
+   if (!attr)
+           return;
+   fprintf(f, "\n  %s %d", name, ntohl(rta_getattr_u32(attr)));
+}
+

Why short circuit, just change the order:

      if (attr)
              fprintf(f, "\n  %s %s", name, ntohl(rta_getattr_u32(attr));

You might also want to introduce rta_getattr_be32()

ack

quoted

Please change, retest and resubmit both patches.

ack

Thanks for reviewing,
Amir