[ovs-dev] [PATCH RFC] ofproto: Allow multiple array elements in PORT_DESC request body

Simon Horman horms at verge.net.au
Thu Dec 12 00:36:03 UTC 2013


Signed-off-by: Simon Horman <horms at verge.net.au>

---

This change is also needed for several other stats request messages.
I have posted this as an RFC before proceeding with updating
the decoders for other stats request messages.
---
 lib/ofp-msgs.h    |  4 ++--
 lib/ofp-print.c   | 23 +++++++++++++++--------
 lib/ofp-util.c    | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 lib/ofp-util.h    |  4 ++--
 ofproto/ofproto.c | 38 ++++++++++++++++++++++++--------------
 tests/ofproto.at  | 36 ++++++++++++++++++++++++++++++++++++
 6 files changed, 126 insertions(+), 33 deletions(-)

diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
index 26fd6a3..ea4b37d 100644
--- a/lib/ofp-msgs.h
+++ b/lib/ofp-msgs.h
@@ -281,9 +281,9 @@ enum ofpraw {
     /* OFPST 1.3 (3): struct ofp13_table_stats[]. */
     OFPRAW_OFPST13_TABLE_REPLY,
 
-    /* OFPST 1.0 (4): struct ofp10_port_stats_request. */
+    /* OFPST 1.0 (4): struct ofp10_port_stats_request[]. */
     OFPRAW_OFPST10_PORT_REQUEST,
-    /* OFPST 1.1+ (4): struct ofp11_port_stats_request. */
+    /* OFPST 1.1+ (4): struct ofp11_port_stats_request[]. */
     OFPRAW_OFPST11_PORT_REQUEST,
 
     /* OFPST 1.0 (4): struct ofp10_port_stats[]. */
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 13705d0..ffeaf12 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1515,16 +1515,23 @@ static void
 ofp_print_ofpst_port_request(struct ds *string, const struct ofp_header *oh)
 {
     ofp_port_t ofp10_port;
-    enum ofperr error;
+    struct ofpbuf b;
 
-    error = ofputil_decode_port_stats_request(oh, &ofp10_port);
-    if (error) {
-        ofp_print_error(string, error);
-        return;
-    }
+    ofpbuf_use_const(&b, oh, ntohs(oh->length));
+    for (;;) {
+        int error;
 
-    ds_put_cstr(string, " port_no=");
-    ofputil_format_port(ofp10_port, string);
+        error = ofputil_decode_port_stats_request(&b, &ofp10_port);
+        if (error) {
+            if (error != EOF) {
+                ofp_print_error(string, error);
+            }
+            return;
+        }
+
+        ds_put_cstr(string, " port_no=");
+        ofputil_format_port(ofp10_port, string);
+    }
 }
 
 static void
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 7fc4c7c..a7c3769 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5739,23 +5739,58 @@ ofputil_decode_port_stats(struct ofputil_port_stats *ps, struct ofpbuf *msg)
     return OFPERR_OFPBRC_BAD_LEN;
 }
 
-/* Parse a port status request message into a 16 bit OpenFlow 1.0
+/* Converts an OFPST_PORT_DESC 'msg' into a 16 bit OpenFlow 1.0
  * port number and stores the latter in '*ofp10_port'.
- * Returns 0 if successful, otherwise an OFPERR_* number. */
-enum ofperr
-ofputil_decode_port_stats_request(const struct ofp_header *request,
+ *
+ * Multiple OFPST_PORT_DESC requests can be packed into a single
+ * OpenFlow message.  Calling this function multiple times for a single 'msg'
+ * iterates through the replies.  The caller must initially leave 'msg''s layer
+ * pointers null and not modify them between calls.
+ *
+ * Returns 0 if successful, EOF if no replies were left in this 'msg',
+ * otherwise an OFPERR_* number. */
+int
+ofputil_decode_port_stats_request(struct ofpbuf *msg,
                                   ofp_port_t *ofp10_port)
 {
-    switch ((enum ofp_version)request->version) {
+    const struct ofp_header *oh;
+    enum ofperr error;
+    enum ofpraw raw;
+
+    error = (msg->l2
+             ? ofpraw_decode(&raw, msg->l2)
+             : ofpraw_pull(&raw, msg));
+    if (error) {
+        return error;
+    }
+    oh = msg->l2;
+
+    if (!msg->size) {
+        return EOF;
+    }
+
+    switch ((enum ofp_version)oh->version) {
     case OFP13_VERSION:
     case OFP12_VERSION:
     case OFP11_VERSION: {
-        const struct ofp11_port_stats_request *psr11 = ofpmsg_body(request);
+        const struct ofp11_port_stats_request *psr11;
+
+        psr11 = ofpbuf_try_pull(msg, sizeof *psr11);
+        if (!psr11) {
+            goto trailing_garbage;
+        }
+
         return ofputil_port_from_ofp11(psr11->port_no, ofp10_port);
     }
 
     case OFP10_VERSION: {
-        const struct ofp10_port_stats_request *psr10 = ofpmsg_body(request);
+        const struct ofp10_port_stats_request *psr10;
+
+        psr10 = ofpbuf_try_pull(msg, sizeof *psr10);
+        if (!psr10) {
+            goto trailing_garbage;
+        }
+
         *ofp10_port = u16_to_ofp(ntohs(psr10->port_no));
         return 0;
     }
@@ -5763,6 +5798,11 @@ ofputil_decode_port_stats_request(const struct ofp_header *request,
     default:
         NOT_REACHED();
     }
+
+trailing_garbage:
+    VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_PORT_DESC request has "
+                 "%"PRIuSIZE" leftover bytes at end", msg->size);
+    return OFPERR_OFPBRC_BAD_LEN;
 }
 
 /* Frees all of the "struct ofputil_bucket"s in the 'buckets' list. */
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index fef85e0..726cd3e 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -918,8 +918,8 @@ void ofputil_append_port_stat(struct list *replies,
                               const struct ofputil_port_stats *ops);
 size_t ofputil_count_port_stats(const struct ofp_header *);
 int ofputil_decode_port_stats(struct ofputil_port_stats *, struct ofpbuf *msg);
-enum ofperr ofputil_decode_port_stats_request(const struct ofp_header *request,
-                                              ofp_port_t *ofp10_port);
+int ofputil_decode_port_stats_request(struct ofpbuf *request,
+                                      ofp_port_t *ofp10_port);
 
 struct ofputil_queue_stats_request {
     ofp_port_t port_no;           /* OFPP_ANY means "all ports". */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3a60328..3a07958 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3151,23 +3151,33 @@ handle_port_stats_request(struct ofconn *ofconn,
     struct ofproto *p = ofconn_get_ofproto(ofconn);
     struct ofport *port;
     struct list replies;
-    ofp_port_t port_no;
-    enum ofperr error;
-
-    error = ofputil_decode_port_stats_request(request, &port_no);
-    if (error) {
-        return error;
-    }
+    struct ofpbuf b;
 
     ofpmp_init(&replies, request);
-    if (port_no != OFPP_ANY) {
-        port = ofproto_get_port(p, port_no);
-        if (port) {
-            append_port_stat(port, &replies);
+
+    ofpbuf_use_const(&b, request, ntohs(request->length));
+    for (;;) {
+        ofp_port_t port_no;
+        int error;
+
+        error = ofputil_decode_port_stats_request(&b, &port_no);
+        if (error) {
+            if (error == EOF) {
+                break;
+            } else {
+                return error;
+            }
         }
-    } else {
-        HMAP_FOR_EACH (port, hmap_node, &p->ports) {
-            append_port_stat(port, &replies);
+
+        if (port_no != OFPP_ANY) {
+            port = ofproto_get_port(p, port_no);
+            if (port) {
+                append_port_stat(port, &replies);
+            }
+        } else {
+            HMAP_FOR_EACH (port, hmap_node, &p->ports) {
+                append_port_stat(port, &replies);
+            }
         }
     }
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index be7387d..3137dcb 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -2152,3 +2152,39 @@ OFPT_BARRIER_REPLY (OF1.3):
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto - port_desc request with multiple elements (OpenFlow 1.3)])
+AT_KEYWORDS([monitor])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2], [3], [4])
+
+# Start a monitor, use the required protocol version
+ovs-ofctl -O OpenFlow13 monitor br0 --detach --no-chdir --pidfile >monitor.log 2>&1
+AT_CAPTURE_FILE([monitor.log])
+
+# Send OpenFlow13 message (04), OFPT_MULTIPART_REQUEST (12), length (0020), xid (0000000a)
+#                               OFPMP_PORT_STATS (0004), no flags (0000), pad (00000000)
+#                               port_no (0001), pad (00000000)
+#                               port_no (0002), pad (00000000)
+ovs-appctl -t ovs-ofctl ofctl/send "041200200000000a 0004000000000000 0000000100000000 0000000200000000"
+
+ovs-appctl -t ovs-ofctl ofctl/barrier
+
+# Check default setting
+read -r -d '' expected <<'EOF'
+EOF
+
+AT_CHECK([ofctl_strip < monitor.log | STRIP_DURATION], [], [dnl
+send: OFPST_PORT request (OF1.3): port_no=1 port_no=2
+OFPST_PORT reply (OF1.3): 2 ports
+  port  1: rx pkts=0, bytes=0, drop=0, errs=0, frame=0, over=0, crc=0
+           tx pkts=0, bytes=0, drop=0, errs=0, coll=0
+           duration=?s
+  port  2: rx pkts=0, bytes=0, drop=0, errs=0, frame=0, over=0, crc=0
+           tx pkts=0, bytes=0, drop=0, errs=0, coll=0
+           duration=?s
+OFPT_BARRIER_REPLY (OF1.3):
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
-- 
1.8.4




More information about the dev mailing list