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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
