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:
(Sending again since I just discovered that Google Inbox is adding an
HTML part...)
>> 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
>
>>
>> > +
>> > + 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
