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
> +{
> + 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?
> +
> + 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);
> +}
> +
> 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?
> +{
> + 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()
Please change, retest and resubmit both patches.
