On 05/06/2017 20:36, Joe Stringer wrote:
On 3 June 2017 at 22:22, Roi Dayan <r...@mellanox.com> wrote:


On 01/06/2017 20:53, Joe Stringer wrote:

On 1 June 2017 at 07:39, Roi Dayan <r...@mellanox.com> wrote:



On 31/05/2017 03:50, Joe Stringer wrote:


On 28 May 2017 at 04:59, Roi Dayan <r...@mellanox.com> wrote:


Add tc helper functions to query and manipulate the flower classifier.

Signed-off-by: Paul Blakey <pa...@mellanox.com>
Signed-off-by: Roi Dayan <r...@mellanox.com>
---



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.


ok

<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?


we want to convert here jiffies to ms and used some default value.
as Flavio suggested maybe do it in a macro?
later I saw in netdev-linux.c the functions read_psched() and
tc_ticks_to_bytes(). can do a followup commit to refactor those
somewhere else and use them in tc.c as well.


Please do.


just to be clear here, is it ok to use a macro for this series
and do the followup commit after this series?

That sounds reasonable.

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


as eth_type is ove_be16 but tc_make_handle wants unsigned int.
we get a compilation warning from sparse about incorrect type.
this is to avoid sparse from failing.


I see. Should it be unsigned int, then? Rather than casting a
big-endian value to store a host-endian variable of the same size?


it's not unsigned int to follow up with the rest of the user space code
that use ovs_be16 for eth_type.
also we are using match_set_dl_type() that expect ovs_be16.

The rest of the code stores a big-endian eth_type in an ovs_be16, or
host byte-order version in uint16_t. The thing I found surprising in
this code was that the big endian version of the ethertype was being
placed into a uint16_t without converting to host byte order. In the
end, I think what we're trying to achieve is that the second parameter
to tc_make_handle() should be a big-endian ethertype but sparse would
complain if we passed it directly (because tc_make_handle()'s second
argument type isn't ovs_be16). So OVS_FORCE is necessary, though I
would have expected it to be used inside the arguments of
tc_make_handle().


maybe. that should also apply to tc_get_minor() to return
ovs_be16 instead of uint.
in parsing back from netlink to tc_flower we do:
flower->key.eth_type = (OVS_FORCE ovs_be16) tc_get_minor(tc->tcm_info);

but a lot of callers use it for vlog and use %u format.
so changing it will probably also cause the compiler to complain.

I'll skip this for V10 as I don't think this is critical
and we can do a commit later to update to what will be agreed on
and update all callers if necessary.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to