lib: simplify flow_extract() API

Change the flow_extract() API to accept struct pkt_metadata,
instead of individual metadata fields. It will make the API more
logical and easier to maintain when we need to expand metadata
down the road.

Signed-off-by: Andy Zhou <azhou@nicira.com>
Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>¬

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d17fa05..5897f8b 100644 (file)
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1454,8 +1454,7 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
     }
 
     /* Extract flow key. */
-    flow_extract(execute->packet, md->skb_priority, md->pkt_mark, &md->tunnel,
-                 (union flow_in_port *)&md->in_port, &key);
+    flow_extract(execute->packet, md, &key);
 
     ovs_rwlock_rdlock(&dp->port_rwlock);
     dp_netdev_execute_actions(dp, &key, execute->packet, md, execute->actions,
@@ -1627,8 +1626,8 @@ dp_forwarder_main(void *f_)
                     if (!error) {
                         struct pkt_metadata md
                             = PKT_METADATA_INITIALIZER(port->port_no);
-                        dp_netdev_port_input(dp, &packet, &md);
 
+                        dp_netdev_port_input(dp, &packet, &md);
                         received_anything = true;
                     } else if (error != EAGAIN && error != EOPNOTSUPP) {
                         static struct vlog_rate_limit rl
@@ -1728,8 +1727,7 @@ dp_netdev_port_input(struct dp_netdev *dp, struct ofpbuf *packet,
     if (packet->size < ETH_HEADER_LEN) {
         return;
     }
-    flow_extract(packet, md->skb_priority, md->pkt_mark, &md->tunnel,
-                 (union flow_in_port *)&md->in_port, &key);
+    flow_extract(packet, md, &key);
     netdev_flow = dp_netdev_lookup_flow(dp, &key);
     if (netdev_flow) {
         struct dp_netdev_actions *actions;

diff --git a/lib/flow.c b/lib/flow.c
index e7fe4d3..82d6729 100644 (file)
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -35,6 +35,7 @@
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
 #include "packets.h"
+#include "odp-util.h"
 #include "random.h"
 #include "unaligned.h"
 
@@ -361,8 +362,7 @@ invalid:
 
 }
 
-/* Initializes 'flow' members from 'packet', 'skb_priority', 'tnl', and
- * 'in_port'.
+/* Initializes 'flow' members from 'packet' and 'md'
  *
  * Initializes 'packet' header pointers as follows:
  *
@@ -381,8 +381,7 @@ invalid:
  *      present and has a correct length, and otherwise NULL.
  */
 void
-flow_extract(struct ofpbuf *packet, uint32_t skb_priority, uint32_t pkt_mark,
-             const struct flow_tnl *tnl, const union flow_in_port *in_port,
+flow_extract(struct ofpbuf *packet, const struct pkt_metadata *md,
              struct flow *flow)
 {
     struct ofpbuf b = *packet;
@@ -392,15 +391,14 @@ flow_extract(struct ofpbuf *packet, uint32_t skb_priority, uint32_t pkt_mark,
 
     memset(flow, 0, sizeof *flow);
 
-    if (tnl) {
-        ovs_assert(tnl != &flow->tunnel);
-        flow->tunnel = *tnl;
-    }
-    if (in_port) {
-        flow->in_port = *in_port;
+    if (md) {
+        flow->tunnel = md->tunnel;
+        if (md->in_port.odp_port != ODPP_NONE) {
+            flow->in_port = md->in_port;
+        };
+        flow->skb_priority = md->skb_priority;
+        flow->pkt_mark = md->pkt_mark;
     }
-    flow->skb_priority = skb_priority;
-    flow->pkt_mark = pkt_mark;
 
     packet->l2   = b.data;
     packet->l2_5 = NULL;

diff --git a/lib/flow.h b/lib/flow.h
index 3109a84..8165bcf 100644 (file)
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -32,6 +32,7 @@ struct ds;
 struct flow_wildcards;
 struct minimask;
 struct ofpbuf;
+struct pkt_metadata;
 
 /* This sequence number should be incremented whenever anything involving flows
  * or the wildcarding of flows changes.  This will cause build assertion
@@ -72,8 +73,8 @@ struct flow_tnl {
  * numbers and other times datapath (dpif) port numbers.  This union allows
  * access to both. */
 union flow_in_port {
-    ofp_port_t ofp_port;
     odp_port_t odp_port;
+    ofp_port_t ofp_port;
 };
 
 /* Maximum number of supported MPLS labels. */
@@ -173,8 +174,7 @@ struct flow_metadata {
     ofp_port_t in_port;              /* OpenFlow port or zero. */
 };
 
-void flow_extract(struct ofpbuf *, uint32_t priority, uint32_t mark,
-                  const struct flow_tnl *, const union flow_in_port *in_port,
+void flow_extract(struct ofpbuf *, const struct pkt_metadata *md,
                   struct flow *);
 
 void flow_zero_wildcards(struct flow *, const struct flow_wildcards *);

diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index 8efbce1..5620990 100644 (file)
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -556,7 +556,6 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
 
     struct ofpbuf pkt;
     struct flow flow;
-    union flow_in_port in_port_;
 
     error = ofputil_decode_packet_in(&pi, oh);
     if (error) {
@@ -574,8 +573,8 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh)
 
     /* Extract flow data from 'opi' into 'flow'. */
     ofpbuf_use_const(&pkt, pi.packet, pi.packet_len);
-    in_port_.ofp_port = pi.fmd.in_port;
-    flow_extract(&pkt, 0, 0, NULL, &in_port_, &flow);
+    flow_extract(&pkt, NULL, &flow);
+    flow.in_port.ofp_port = pi.fmd.in_port;
     flow.tunnel.tun_id = pi.fmd.tun_id;
 
     /* Choose output port. */

diff --git a/lib/odp-util.c b/lib/odp-util.c
index e20564f..d86dbbb 100644 (file)
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2646,8 +2646,8 @@ odp_key_from_pkt_metadata(struct ofpbuf *buf, const struct pkt_metadata *md)
 
     /* Add an ingress port attribute if 'odp_in_port' is not the magical
      * value "ODPP_NONE". */
-    if (md->in_port != ODPP_NONE) {
-        nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, md->in_port);
+    if (md->in_port.odp_port != ODPP_NONE) {
+        nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, md->in_port.odp_port);
     }
 }
 
@@ -2662,8 +2662,7 @@ odp_key_to_pkt_metadata(const struct nlattr *key, size_t key_len,
         1u << OVS_KEY_ATTR_SKB_MARK | 1u << OVS_KEY_ATTR_TUNNEL |
         1u << OVS_KEY_ATTR_IN_PORT;
 
-    memset(md, 0, sizeof *md);
-    md->in_port = ODPP_NONE;
+    *md = PKT_METADATA_INITIALIZER(ODPP_NONE);
 
     NL_ATTR_FOR_EACH (nla, left, key, key_len) {
         uint16_t type = nl_attr_type(nla);
@@ -2690,7 +2689,7 @@ odp_key_to_pkt_metadata(const struct nlattr *key, size_t key_len,
                 wanted_attrs &= ~(1u << OVS_KEY_ATTR_TUNNEL);
             }
         } else if (type == OVS_KEY_ATTR_IN_PORT) {
-            md->in_port = nl_attr_get_odp_port(nla);
+            md->in_port.odp_port = nl_attr_get_odp_port(nla);
             wanted_attrs &= ~(1u << OVS_KEY_ATTR_IN_PORT);
         }
 

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 4c89b36..06e64f6 100644 (file)
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -46,6 +46,7 @@
 #include "packets.h"
 #include "type-props.h"
 #include "unaligned.h"
+#include "odp-util.h"
 #include "util.h"
 
 static void ofp_print_queue_name(struct ds *string, uint32_t port);
@@ -58,11 +59,12 @@ char *
 ofp_packet_to_string(const void *data, size_t len)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
+    const struct pkt_metadata md = PKT_METADATA_INITIALIZER(ODPP_NONE);
     struct ofpbuf buf;
     struct flow flow;
 
     ofpbuf_use_const(&buf, data, len);
-    flow_extract(&buf, 0, 0, NULL, NULL, &flow);
+    flow_extract(&buf, &md, &flow);
     flow_format(&ds, &flow);
 
     if (buf.l7) {

diff --git a/lib/packets.c b/lib/packets.c
index 7238f42..3f7d6eb 100644 (file)
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -29,6 +29,7 @@
 #include "dynamic-string.h"
 #include "ofpbuf.h"
 #include "ovs-thread.h"
+#include "odp-util.h"
 #include "unaligned.h"
 
 const struct in6_addr in6addr_exact = IN6ADDR_EXACT_INIT;
@@ -991,3 +992,23 @@ packet_format_tcp_flags(struct ds *s, uint16_t tcp_flags)
         ds_put_cstr(s, "[800]");
     }
 }
+
+void pkt_metadata_init(struct pkt_metadata *md, const struct flow_tnl *tnl,
+                            const uint32_t skb_priority,
+                            const uint32_t pkt_mark,
+                            const union flow_in_port *in_port)
+{
+
+    tnl ? memcpy(&md->tunnel, tnl, sizeof(md->tunnel))
+        : memset(&md->tunnel, 0, sizeof(md->tunnel));
+
+    md->skb_priority = skb_priority;
+    md->pkt_mark = pkt_mark;
+    md->in_port.odp_port = in_port ? in_port->odp_port : ODPP_NONE;
+}
+
+void pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow)
+{
+    pkt_metadata_init(md, &flow->tunnel, flow->skb_priority,
+                           flow->pkt_mark, &flow->in_port);
+}

diff --git a/lib/packets.h b/lib/packets.h
index 1855a1c..e6b3303 100644 (file)
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -36,11 +36,17 @@ struct pkt_metadata {
     struct flow_tnl tunnel;     /* Encapsulating tunnel parameters. */
     uint32_t skb_priority;      /* Packet priority for QoS. */
     uint32_t pkt_mark;          /* Packet mark. */
-    odp_port_t in_port;         /* Input port. */
+    union flow_in_port in_port; /* Input port. */
 };
 
 #define PKT_METADATA_INITIALIZER(PORT) \
-    (struct pkt_metadata){ { 0, 0, 0, 0, 0, 0}, 0, 0, (PORT) }
+    (struct pkt_metadata){ { 0, 0, 0, 0, 0, 0}, 0, 0, {(PORT)} }
+
+void pkt_metadata_init(struct pkt_metadata *md, const struct flow_tnl *tnl,
+                            const uint32_t skb_priority,
+                            const uint32_t pkt_mark,
+                            const union flow_in_port *in_port);
+void pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow);
 
 bool dpid_from_string(const char *s, uint64_t *dpidp);
 

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index c50694b..7dbd7f7 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -992,9 +992,10 @@ handle_upcalls(struct handler *handler, struct list *upcalls)
         type = classify_upcall(upcall);
         if (type == MISS_UPCALL) {
             uint32_t hash;
+            struct pkt_metadata md;
 
-            flow_extract(packet, flow.skb_priority, flow.pkt_mark,
-                         &flow.tunnel, &flow.in_port, &miss->flow);
+            pkt_metadata_from_flow(&md, &flow);
+            flow_extract(packet, &md, &miss->flow);
 
             hash = flow_hash(&miss->flow, 0);
             existing_miss = flow_miss_find(&misses, ofproto, &miss->flow,

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 89d92af..eb4931e 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3184,12 +3184,11 @@ xlate_send_packet(const struct ofport_dpif *ofport, struct ofpbuf *packet)
     struct xport *xport;
     struct ofpact_output output;
     struct flow flow;
-    union flow_in_port in_port_;
 
     ofpact_init(&output.ofpact, OFPACT_OUTPUT, sizeof output);
     /* Use OFPP_NONE as the in_port to avoid special packet processing. */
-    in_port_.ofp_port = OFPP_NONE;
-    flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
+    flow_extract(packet, NULL, &flow);
+    flow.in_port.ofp_port = OFPP_NONE;
 
     ovs_rwlock_rdlock(&xlate_rwlock);
     xport = xport_lookup(ofport);

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index af21dff..8c43ee9 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2997,7 +2997,7 @@ ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
     execute.md.tunnel = flow->tunnel;
     execute.md.skb_priority = flow->skb_priority;
     execute.md.pkt_mark = flow->pkt_mark;
-    execute.md.in_port = ofp_port_to_odp_port(ofproto, in_port);
+    execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
     execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
 
     error = dpif_execute(ofproto->backer->dpif, &execute);
@@ -3784,11 +3784,14 @@ parse_flow_and_packet(int argc, const char *argv[],
             flow_compose(packet, flow);
         } else {
             union flow_in_port in_port = flow->in_port;
+            struct pkt_metadata md;
 
             /* Use the metadata from the flow and the packet argument
              * to reconstruct the flow. */
-            flow_extract(packet, flow->skb_priority, flow->pkt_mark, NULL,
-                         &in_port, flow);
+            pkt_metadata_init(&md, NULL, flow->skb_priority,
+                                   flow->pkt_mark, &in_port);
+
+            flow_extract(packet, &md, flow);
         }
     }
 

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7117e1f..19e7091 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2713,11 +2713,10 @@ run_rule_executes(struct ofproto *ofproto)
 
     guarded_list_pop_all(&ofproto->rule_executes, &executes);
     LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
-        union flow_in_port in_port_;
         struct flow flow;
 
-        in_port_.ofp_port = e->in_port;
-        flow_extract(e->packet, 0, 0, NULL, &in_port_, &flow);
+        flow_extract(e->packet, NULL, &flow);
+        flow.in_port.ofp_port = e->in_port;
         ofproto->ofproto_class->rule_execute(e->rule, &flow, e->packet);
 
         rule_execute_destroy(e);
@@ -2926,7 +2925,6 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     uint64_t ofpacts_stub[1024 / 8];
     struct ofpbuf ofpacts;
     struct flow flow;
-    union flow_in_port in_port_;
     enum ofperr error;
 
     COVERAGE_INC(ofproto_packet_out);
@@ -2960,8 +2958,8 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     }
 
     /* Verify actions against packet, then send packet if successful. */
-    in_port_.ofp_port = po.in_port;
-    flow_extract(payload, 0, 0, NULL, &in_port_, &flow);
+    flow_extract(payload, NULL, &flow);
+    flow.in_port.ofp_port = po.in_port;
     error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len);
     if (!error) {
         error = p->ofproto_class->packet_out(p, payload, &flow,

diff --git a/tests/test-flows.c b/tests/test-flows.c
index 2910035..a498142 100644 (file)
--- a/tests/test-flows.c
+++ b/tests/test-flows.c
@@ -58,7 +58,6 @@ main(int argc OVS_UNUSED, char *argv[])
         struct ofp10_match extracted_match;
         struct match match;
         struct flow flow;
-        union flow_in_port in_port_;
         n++;
 
         retval = ovs_pcap_read(pcap, &packet, NULL);
@@ -68,8 +67,9 @@ main(int argc OVS_UNUSED, char *argv[])
             ovs_fatal(retval, "error reading pcap file");
         }
 
-        in_port_.ofp_port = u16_to_ofp(1);
-        flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
+        flow_extract(packet, NULL, &flow);
+        flow.in_port.ofp_port = u16_to_ofp(1);
+
         match_wc_init(&match, &flow);
         ofputil_match_to_ofp10_match(&match, &extracted_match);
 

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 4ab9ca4..e62e646 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1864,12 +1864,13 @@ ofctl_ofp_parse_pcap(int argc OVS_UNUSED, char *argv[])
         struct ofpbuf *packet;
         long long int when;
         struct flow flow;
+        const struct pkt_metadata md = PKT_METADATA_INITIALIZER(ODPP_NONE);
 
         error = ovs_pcap_read(file, &packet, &when);
         if (error) {
             break;
         }
-        flow_extract(packet, 0, 0, NULL, NULL, &flow);
+        flow_extract(packet, &md, &flow);
         if (flow.dl_type == htons(ETH_TYPE_IP)
             && flow.nw_proto == IPPROTO_TCP
             && (is_openflow_port(flow.tp_src, argv + 2) ||
@@ -3208,6 +3209,7 @@ ofctl_parse_pcap(int argc OVS_UNUSED, char *argv[])
     for (;;) {
         struct ofpbuf *packet;
         struct flow flow;
+        const struct pkt_metadata md = PKT_METADATA_INITIALIZER(ODPP_NONE);
         int error;
 
         error = ovs_pcap_read(pcap, &packet, NULL);
@@ -3217,7 +3219,7 @@ ofctl_parse_pcap(int argc OVS_UNUSED, char *argv[])
             ovs_fatal(error, "%s: read failed", argv[1]);
         }
 
-        flow_extract(packet, 0, 0, NULL, NULL, &flow);
+        flow_extract(packet, &md, &flow);
         flow_print(stdout, &flow);
         putchar('\n');
         ofpbuf_delete(packet);