[ovs-dev] [learning v2 15/19] Implement new "learn" action.

[Ethan Jackson]

I get the following compiler warning:

lib/learn.c: In function 'learn_format':
lib/learn.c:463:32: error: variable 'src_field' set but not used
[-Werror=unused-but-set-variable]

>     ovs-vsctl del-controller br0
>     ovs-ofctl del-flows br0
>     ovs-ofctl add-flow br0 "table=0 actions=learn(table=1, hard_timeout=10, \
>     	NXM_OF_VLAN_TCI[0..11], NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], \
>     	output:NXM_OF_IN_PORT[]), resubmit(,1)"
>     ovs-ofctl add-flow br0 "table=1 priority=0 actions=flood"
>
>     You can then dump the MAC learning table with:
>
>     ovs-ofctl dump-flows br0 table=1

This seems nifty.  Perhaps we should put this in the actual documentation
somewhere.  Maybe in the NXAST_LEARN documentation, or ovs-ofctl?

+      - Added an OpenFlow extension for flexible MAC learning.

I would just say "learning" not "MAC learning".  It's way more powerful than
that.

+ * The flow_mod_spec destination spec for 'dst' of 2 (when 'src' is 0) is
+ * empty.  It has the following meaning:
+ *
+ *   - The flow_mod_spec specifies an OFPAT_OUTPUT action for the new flow.
+ *     The new flow outputs to the OpenFlow port specified by the source field.
+ *     Of the special output ports with value OFPP_MAX or larger, OFPP_IN_PORT,
+ *     OFPP_FLOOD, OFPP_LOCAL, and OFPP_ALL are supported.  Other special ports
+ *     may not be used.

Do we need this?  I feel like it's going to get stale if unused.  I'd prefer
the action be orthogonal, users can just write the OpenFlow port to a register
and output to that register.

+ * These examples could work with various nx_action_learn parameters.  Typical
+ * values would be hard_timeout=OFP_FLOW_PERMANENT, hard_timeout=60,
+ * priority=OFP_DEFAULT_PRIORITY, flags=0, table_id=10.

Did you intend to specify hard_timeout twice in this sentence?

+#define NX_LEARN_DST_RESERVED  (3 << 11)
+#define NX_LEARN_DST_MASK      (3 << 11)

Do you intend to have NX_LEARN_DST_MASK in this patch?  If so, do we need
NX_LEARN_DST_RESERVED?  I'm slightly confused that they have the same value.

+static bool
+is_all_zeros(const uint8_t *field, size_t length)
+{
+    size_t i;
+
+    for (i = 0; i < length; i++) {
+        if (field[i] != 0x00) {
+            return false;
+        }
+    }
+    return true;
+}

This same function is in meta-flow along with it's cousin is_all_ones().
Perhaps we should pull it into util.  Seems generally useful.

+int
+learn_check(const struct nx_action_learn *learn, const struct flow *flow)
+{
+    struct cls_rule rule;
+    const void *p, *end;
+
+    cls_rule_init_catchall(&rule, 0);
+
+    if (learn->flags & ~htons(OFPFF_SEND_FLOW_REM)
+        || !is_all_zeros(learn->pad, sizeof learn->pad)
+        || learn->table_id == 0xff) {
+        return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT);
+    }
+
+    end = (char *) learn + ntohs(learn->len);
+    for (p = learn + 1; p != end; ) {

Should we check that learn->len >= sizeof learn before the for loop?

+            if (dst_type == NX_LEARN_DST_MATCH
+                && src_type == NX_LEARN_SRC_IMMEDIATE) {
+                mf_set_subfield(nxm_field_to_mf_field(ntohl(dst_field)), value,
+                                dst_ofs, n_bits, &rule);
+            }

It's not clear to me why we we need to make this call to mf_set_subfield().  If
there's a good reason for it, it could benefit from a comment.  If we don't need
it, then we could get rid of the cls_rule in this function, and the computation
of 'value'.

+        /* Check that the arguments don't overrun the end of the action. */
+        min_len = 0;
+        if (src_type == NX_LEARN_SRC_FIELD) {
+            min_len += sizeof(ovs_be32); /* src_header */
+            min_len += sizeof(ovs_be16); /* src_ofs */
+        } else {
+            min_len += 2 * DIV_ROUND_UP(n_bits, 16);
+        }
+        if (dst_type == NX_LEARN_DST_MATCH ||
+            dst_type == NX_LEARN_DST_LOAD) {
+            min_len += sizeof(ovs_be32); /* dst_header */
+            min_len += sizeof(ovs_be16); /* dst_ofs */
+        }

I wonder if this should be factored out into a helper since it's done twice.  It
could take 'header' as an argument.

+        switch (src_type | dst_type) {

Awesome! I had no idea you could do this.

+#ifndef LEARN_H
+#define LEARN_H 1
+
+#include <stdint.h>

I don't think this include is necessary.

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index a97a0e3..6705ae4 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -16,6 +16,7 @@

 #include <config.h>
 #include "ofp-print.h"
+#include <ctype.h>
 #include <errno.h>
 #include <inttypes.h>
 #include <netinet/icmp6.h>

Is this include addition necessary?

Ethan