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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Nov 30, 2016 at 08:38:24AM CET, amirva@gmail.com wrote:
>On Wed, Nov 30, 2016 at 9:17 AM Amir Vadai <amir@vadai.me> wrote:
>
>> On Tue, Nov 29, 2016 at 07:26:58PM -0800, Stephen Hemminger wrote:
>> > The overall design is fine, just a couple nits with the code.
>> >
>> > > 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
>> >
>> > >
>> > > +static int flower_parse_key_id(char *str, int type, struct nlmsghdr
>> *n)
>> >
>> > str is not modified, therefore use: const char *str
>> ack
>>
>> >
>> > > +{
>> > > +   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.


>
>
>> >
>> > > +
>> > > +   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.
>>
>> >
>> > > +}
>> > > +
>> > >  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,
>> > >                             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,
>> > > +
>> TCA_FLOWER_KEY_ENC_IPV4_DST_MASK,
>> > > +
>> TCA_FLOWER_KEY_ENC_IPV6_DST,
>> > > +
>> TCA_FLOWER_KEY_ENC_IPV6_DST_MASK,
>> > > +                                              n);
>> > > +                   if (ret < 0) {
>> > > +                           fprintf(stderr, "Illegal
>> \"enc_dst_ip\"\n");
>> > > +                           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,
>> > > +
>> TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK,
>> > > +
>> TCA_FLOWER_KEY_ENC_IPV6_SRC,
>> > > +
>> TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK,
>> > > +                                              n);
>> > > +                   if (ret < 0) {
>> > > +                           fprintf(stderr, "Illegal
>> \"enc_src_ip\"\n");
>> > > +                           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);
>> > > +                   if (ret < 0) {
>> > > +                           fprintf(stderr, "Illegal
>> \"enc_key_id\"\n");
>> > > +                           return -1;
>> > > +                   }
>> > >             } else if (matches(*argv, "action") == 0) {
>> > >                     NEXT_ARG();
>> > >                     ret = parse_action(&argc, &argv, TCA_FLOWER_ACT,
>> n);
>> > > @@ -509,6 +561,14 @@ static void flower_print_port(FILE *f, char
>> *name, __u8 ip_proto,
>> > >     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
>>
>> >
>> >
>> > > +{
>> > > +   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
>>
>> >
>> > Please change, retest and resubmit both patches.
>> ack
>>
>> Thanks for reviewing,
>> Amir
>>