On 28 May 2017 at 04:59, Roi Dayan <[email protected]> wrote:
> Add tc helper functions to query and manipulate the flower classifier.
>
> Signed-off-by: Paul Blakey <[email protected]>
> Signed-off-by: Roi Dayan <[email protected]>
> ---
Again is this co-authored? utilities/checkpatch.py checks for stuff like this.
I didn't go through all of the enums and make sure they're all
covered, correct types, etc.
<snip>
> + [TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK] = { .type = NL_A_U32, .optional =
> true, },
> + [TCA_FLOWER_KEY_ENC_IPV4_DST_MASK] = { .type = NL_A_U32, .optional =
> true, },
Line lengths.
<snip>
> +static void
> +nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
> +{
> + unsigned long long int lastuse = tm->lastuse * 10;
Where does the 10 come from? What does it mean? Should we have a #define for it?
<snip>
> + stats_basic = stats_attrs[TCA_STATS_BASIC];
> + bs = nl_attr_get_unspec(stats_basic, sizeof *bs);
> +
> + stats->n_packets.lo = bs->packets;
> + stats->n_packets.hi = 0;
> + stats->n_bytes.hi = bs->bytes >> 32;
> + stats->n_bytes.lo = bs->bytes & 0x00000000FFFFFFFF;
Should this use put_32aligned_u64()?
> +
> + return 0;
> +}
> +
> +#define TCA_ACT_MIN_PRIO 1
Stray define - Shouldn't this be in linux/pkt_cls.h?
<snip>
> + if (!nl_policy_parse(reply, NLMSG_HDRLEN + sizeof *tc,
> + tca_policy, ta, ARRAY_SIZE(ta))) {
> + VLOG_ERR_RL(&error_rl, "failed to parse tca policy");
> + return EPROTO;
> + }
> +
> + kind = nl_attr_get_string(ta[TCA_KIND]);
> + if (strcmp(kind, "flower")) {
> + VLOG_ERR_RL(&error_rl, "failed to parse filter: not flower");
Printing the filter kind in the log message might be useful for
debugging purposes.
> +
> + tcmsg = tc_make_request(ifindex, RTM_GETTFILTER, NLM_F_DUMP, &request);
> + tcmsg->tcm_parent = tc_make_handle(0xffff, 0);
Do TC people usually define handles in terms of these specific
integers? I wonder if a subsequent follow-up patch would improve
readability by #defining these tc_make_handle(0xffff, 0) numbers to
show what they actually mean - like ingress, root.
> +int
> +tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
> + struct tc_flower *flower)
> +{
> + struct ofpbuf request;
> + struct tcmsg *tcmsg;
> + struct ofpbuf *reply;
> + int error = 0;
> + size_t basic_offset;
> + uint16_t eth_type = (OVS_FORCE uint16_t) flower->key.eth_type;
Why does this need forcing?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
