[ovs-dev] [PATCH v2] datapath: Refactor actions in terms of match fields.

[Ben Pfaff]

I didn't really read the kernel code.

<linux/openvswitch.h>
---------------------

I think that the intention is that OVS_ACTION_ATTR_{SET,PUSH} contain
exactly one nested attribute, but the comment on it doesn't say that.
It's common to support a bunch of nested attributes, and so I could
see someone trying to set several keys in a single
OVS_ACTION_ATTR_SET, so it's probably a good idea to mention that only
a single field can be set with a single OVS_ACTION_ATTR_{SET,PUSH}.

I think that the new comment should explain each possibility
explicitly, like the comment for enum ovs_flow_attr does.  I believe
that currently it is really talking about OVS_ACTION_ATTR_SET and
OVS_ACTION_ATTR_PUSH, but it doesn't really say so.

The new comment doesn't have the right form for a kerneldoc comment.
It should start with "enum ovs_action_attr - " followed by a brief
description that fits on the first line.  And I think that it should
be just above the enum declaration, so that you should delete the /*
Action types. */ comment and the blank line.

userspace
---------

In dp_netdev_set_dl(), I don't see a benefit to doing the
comparisons.  I think that we can just copy in the new Ethernet source
and destination directly.

This line is wrong (s/udp_dst/udp_src/):
+    if (uh->udp_dst != udp_key->udp_src) {

In execute_set_action(), please assign the nl_attr_type value to an
"enum ovs_key_attr" and then switch on that, adding a case for each
existing OVS_KEY_ATTR_*.  That way, we'll get a warning when we add a
new OVS_KEY_ATTR_* type, to remind us to implement it.

Are OVS_ACTION_ATTR_PUSH and OVS_ACTION_ATTR_POP so simple because we
only support pushing and popping VLAN tags?  Then I'd like an
assertion in there to verify that the inner attribute type is
OVS_KEY_ATTR_8021Q.

{} are needed here around the statements subordinate to "if":
+    case OVS_ACTION_ATTR_POP:
+        if (nl_attr_get_u16(a) == OVS_KEY_ATTR_8021Q)
+            ds_put_cstr(ds, "pop(vlan)");
+        else
+            ds_put_format(ds, "pop(key=%"PRIu16")", nl_attr_get_u16(a));

In facet_account(), q_key can be declared inside the "if" statement
instead of at a higher level.

Please don't give a function in ofproto-dpif.c a name beginning with
nl_, since that prefix belongs to lib/netlink.c.  I think you can
rename nl_msg_put_nested_key() to put_nested_key().  Or it could
reasonably go in odp-util.c under some appropriate name.  In fact, it
seems appropriate to me to put the bulk of the new code in the
commit_*() functions into odp-util.c; ofproto-dpif.c is already too
big and this seems like a clean separation.

I think that the new version of commit_vlan_action() can be slightly
simplified to:

static void
commit_vlan_action(struct action_xlate_ctx *ctx, ovs_be16 new_tci)
{
    struct flow *base = &ctx->base_flow;

    if (base->vlan_tci == new_tci) {
        return;
    }

    if (base->vlan_tci & htons(VLAN_CFI)) {
        nl_msg_put_u16(ctx->odp_actions, OVS_ACTION_ATTR_POP,
                       OVS_KEY_ATTR_8021Q);
    }
    if (new_tci & htons(VLAN_CFI)) {
        struct ovs_key_8021q q_key;

        q_key.q_tpid = htons(ETH_TYPE_VLAN);
        q_key.q_tci = new_tci & ~htons(VLAN_CFI);

        nl_msg_put_nested_key(ctx->odp_actions, OVS_ACTION_ATTR_PUSH,
                              OVS_KEY_ATTR_8021Q, &q_key, sizeof(q_key));
    }
    base->vlan_tci = new_tci;
}

ipv4_key and ipv6_key end with some padding, so it might be a good
idea to memset() them to 0 before initializing.

It looks like commit_set_nw_action() and commit_set_port_action()
forbid setting IPv4 addresses and TCP/UDP ports to 0.  Maybe you want
to check 'base' instead of 'flow' for nonzero?


Thanks,

Ben.