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.
>
> > +
> > + 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
