diff mbox

[ovs-dev,v3,1/5] logical-fields: New header for logical field assignments.

Message ID 1445197962-20874-2-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 18, 2015, 7:52 p.m. UTC 
The original concept for "expr" and "actions" was that they should not need
to know anything about the mapping between physical and logical fields,
that instead everything should be provided via the symbol table.  In
practice this has proven difficult because a couple of actions need to know
about logical fields.  For now, it seems reasonable to put the logical
field mapping into a header of its own.  Later, maybe we'll figure out
whether there's value in a less leaky abstraction.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 ovn/controller/lflow.h     | 22 ++--------------------
 ovn/lib/actions.c          |  4 ++--
 ovn/lib/automake.mk        |  3 ++-
 ovn/lib/logical-fields.h   | 40 ++++++++++++++++++++++++++++++++++++++++
 ovn/ovn-architecture.7.xml |  6 +++---
 5 files changed, 49 insertions(+), 26 deletions(-)
 create mode 100644 ovn/lib/logical-fields.h

Comments

Justin Pettit Oct. 19, 2015, 7:17 a.m. UTC | #1 
> On Oct 18, 2015, at 12:52 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> The original concept for "expr" and "actions" was that they should not need
> to know anything about the mapping between physical and logical fields,
> that instead everything should be provided via the symbol table.  In
> practice this has proven difficult because a couple of actions need to know
> about logical fields.  For now, it seems reasonable to put the logical
> field mapping into a header of its own.  Later, maybe we'll figure out
> whether there's value in a less leaky abstraction.
> 
> Signed-off-by: Ben Pfaff <blp@nicira.com>

Thanks for doing this.  I looked at the changes since the last version, and they look good to me.  I think you have ACKs on everything then.

Acked-by: Justin Pettit <jpettit@nicira.com>

--Justin
Ben Pfaff Oct. 19, 2015, 3:48 p.m. UTC | #2 
On Mon, Oct 19, 2015 at 12:17:04AM -0700, Justin Pettit wrote:
> 
> > On Oct 18, 2015, at 12:52 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > The original concept for "expr" and "actions" was that they should not need
> > to know anything about the mapping between physical and logical fields,
> > that instead everything should be provided via the symbol table.  In
> > practice this has proven difficult because a couple of actions need to know
> > about logical fields.  For now, it seems reasonable to put the logical
> > field mapping into a header of its own.  Later, maybe we'll figure out
> > whether there's value in a less leaky abstraction.
> > 
> > Signed-off-by: Ben Pfaff <blp@nicira.com>
> 
> Thanks for doing this.  I looked at the changes since the last
> version, and they look good to me.  I think you have ACKs on
> everything then.
> 
> Acked-by: Justin Pettit <jpettit@nicira.com>

Thanks, I applied this series to master.
diff mbox

Patch

diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index c3a92f6..4a4fa83 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -13,10 +13,11 @@ 
  * limitations under the License.
  */
 
-
 #ifndef OVN_LFLOW_H
 #define OVN_LFLOW_H 1
 
+#include "ovn/lib/logical-fields.h"
+
 /* Logical_Flow table translation to OpenFlow
  * ==========================================
  *
@@ -54,25 +55,6 @@  struct uuid;
 /* The number of tables for the ingress and egress pipelines. */
 #define LOG_PIPELINE_LEN 16
 
-/* Logical fields.
- *
- * These values are documented in ovn-architecture(7), please update the
- * documentation if you change any of them. */
-#define MFF_LOG_DATAPATH MFF_METADATA /* Logical datapath (64 bits). */
-#define MFF_LOG_CT_ZONE  MFF_REG5     /* Logical conntrack zone (32 bits). */
-#define MFF_LOG_INPORT   MFF_REG6     /* Logical input port (32 bits). */
-#define MFF_LOG_OUTPORT  MFF_REG7     /* Logical output port (32 bits). */
-
-/* Logical registers.
- *
- * Make sure these don't overlap with the logical fields! */
-#define MFF_LOG_REGS \
-    MFF_LOG_REG(MFF_REG0) \
-    MFF_LOG_REG(MFF_REG1) \
-    MFF_LOG_REG(MFF_REG2) \
-    MFF_LOG_REG(MFF_REG3) \
-    MFF_LOG_REG(MFF_REG4)
-
 void lflow_init(void);
 void lflow_run(struct controller_ctx *, struct hmap *flow_table,
                const struct simap *ct_zones);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index aebe5ce..ccf97f0 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -22,6 +22,7 @@ 
 #include "dynamic-string.h"
 #include "expr.h"
 #include "lex.h"
+#include "logical-fields.h"
 #include "ofp-actions.h"
 #include "ofpbuf.h"
 #include "simap.h"
@@ -200,8 +201,7 @@  emit_ct(struct action_context *ctx, bool recirc_next, bool commit)
         ct->recirc_table = NX_CT_RECIRC_NONE;
     }
 
-    /* xxx Should remove hard-coding reg5 if we refactor library. */
-    ct->zone_src.field = mf_from_id(MFF_REG5);
+    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
     ct->zone_src.ofs = 0;
     ct->zone_src.n_bits = 16;
 
diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
index 078e200..9e09352 100644
--- a/ovn/lib/automake.mk
+++ b/ovn/lib/automake.mk
@@ -9,7 +9,8 @@  ovn_lib_libovn_la_SOURCES = \
 	ovn/lib/expr.c \
 	ovn/lib/expr.h \
 	ovn/lib/lex.c \
-	ovn/lib/lex.h
+	ovn/lib/lex.h \
+	ovn/lib/logical-fields.h
 nodist_ovn_lib_libovn_la_SOURCES = \
 	ovn/lib/ovn-nb-idl.c \
 	ovn/lib/ovn-nb-idl.h \
diff --git a/ovn/lib/logical-fields.h b/ovn/lib/logical-fields.h
new file mode 100644
index 0000000..41d42a5
--- /dev/null
+++ b/ovn/lib/logical-fields.h
@@ -0,0 +1,40 @@ 
+/* Copyright (c) 2015 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef OVN_LOGICAL_FIELDS_H
+#define OVN_LOGICAL_FIELDS_H 1
+
+#include "meta-flow.h"
+
+/* Logical fields.
+ *
+ * These values are documented in ovn-architecture(7), please update the
+ * documentation if you change any of them. */
+#define MFF_LOG_DATAPATH MFF_METADATA /* Logical datapath (64 bits). */
+#define MFF_LOG_CT_ZONE  MFF_REG5     /* Logical conntrack zone (32 bits). */
+#define MFF_LOG_INPORT   MFF_REG6     /* Logical input port (32 bits). */
+#define MFF_LOG_OUTPORT  MFF_REG7     /* Logical output port (32 bits). */
+
+/* Logical registers.
+ *
+ * Make sure these don't overlap with the logical fields! */
+#define MFF_LOG_REGS \
+    MFF_LOG_REG(MFF_REG0) \
+    MFF_LOG_REG(MFF_REG1) \
+    MFF_LOG_REG(MFF_REG2) \
+    MFF_LOG_REG(MFF_REG3) \
+    MFF_LOG_REG(MFF_REG4)
+
+#endif /* ovn/lib/logical-fields.h */
diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index 0bf9337..343aa7e 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -626,7 +626,7 @@ 
       A field that denotes the logical datapath through which a packet is being
       processed.
       <!-- Keep the following in sync with MFF_LOG_DATAPATH in
-           ovn/controller/lflow.h. -->
+           ovn/lib/logical-fields.h. -->
       OVN uses the field that OpenFlow 1.1+ simply (and confusingly) calls
       ``metadata'' to store the logical datapath.  (This field is passed across
       tunnels as part of the tunnel key.)
@@ -638,7 +638,7 @@ 
         A field that denotes the logical port from which the packet
         entered the logical datapath.
         <!-- Keep the following in sync with MFF_LOG_INPORT in
-             ovn/controller/lflow.h. -->
+             ovn/lib/logical-fields.h. -->
         OVN stores this in Nicira extension register number 6.
       </p>
 
@@ -659,7 +659,7 @@ 
         leave the logical datapath.  This is initialized to 0 at the
         beginning of the logical ingress pipeline.
         <!-- Keep the following in sync with MFF_LOG_OUTPORT in
-             ovn/controller/lflow.h. -->
+             ovn/lib/logical-fields.h. -->
         OVN stores this in Nicira extension register number 7.
       </p>