Wed, Nov 30, 2016 at 08:38:24AM CET, ami...@gmail.com wrote: >On Wed, Nov 30, 2016 at 9:17 AM Amir Vadai <a...@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 >>