diff mbox series

[ovs-dev,8/8] ofproto: Handle flow monitor requests with multiple parts.

Message ID 20180830200056.15484-8-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev,1/8] vconn: Avoid null dereference on error path. | expand

Commit Message

Ben Pfaff Aug. 30, 2018, 8 p.m. UTC 
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto.c | 84 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 44 insertions(+), 40 deletions(-)

Comments

Ben Pfaff Oct. 12, 2018, 3:55 p.m. UTC | #1 
On Thu, Aug 30, 2018 at 01:00:56PM -0700, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff <blp@ovn.org>

This series still needs a review.
Justin Pettit Jan. 11, 2019, 6:09 p.m. UTC | #2 
> On Aug 30, 2018, at 1:00 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
> ofproto/ofproto.c | 84 +++++++++++++++++++++++++++++--------------------------
> 1 file changed, 44 insertions(+), 40 deletions(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index d65a3fea1559..54f56d9f100e 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6344,49 +6344,60 @@ flow_monitor_delete(struct ofconn *ofconn, uint32_t id)
> }
> 
> static enum ofperr
> -handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
> +handle_flow_monitor_request(struct ofconn *ofconn, const struct ovs_list *msgs)
>     OVS_EXCLUDED(ofproto_mutex)
> {
> ...
> +            if (request.table_id != 0xff

Do you think it would be worth using OFPTT_ALL?

> @@ -8398,7 +8400,7 @@ handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header *oh,
>         return handle_port_desc_stats_request(ofconn, oh);
> 
>     case OFPTYPE_FLOW_MONITOR_STATS_REQUEST:
> -        return handle_flow_monitor_request(ofconn, oh);
> +        OVS_NOT_REACHED();

Grouping this with the OFPTYPE_TABLE_FEATURES_STATS_REQUEST case and adding a comment that they are handled by the multi-part requests might make it clearer why this should not be reached.

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Jan. 16, 2019, 1:10 a.m. UTC | #3 
On Fri, Jan 11, 2019 at 10:09:05AM -0800, Justin Pettit wrote:
> 
> > On Aug 30, 2018, at 1:00 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> > ofproto/ofproto.c | 84 +++++++++++++++++++++++++++++--------------------------
> > 1 file changed, 44 insertions(+), 40 deletions(-)
> > 
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index d65a3fea1559..54f56d9f100e 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -6344,49 +6344,60 @@ flow_monitor_delete(struct ofconn *ofconn, uint32_t id)
> > }
> > 
> > static enum ofperr
> > -handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
> > +handle_flow_monitor_request(struct ofconn *ofconn, const struct ovs_list *msgs)
> >     OVS_EXCLUDED(ofproto_mutex)
> > {
> > ...
> > +            if (request.table_id != 0xff
> 
> Do you think it would be worth using OFPTT_ALL?
> 
> > @@ -8398,7 +8400,7 @@ handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header *oh,
> >         return handle_port_desc_stats_request(ofconn, oh);
> > 
> >     case OFPTYPE_FLOW_MONITOR_STATS_REQUEST:
> > -        return handle_flow_monitor_request(ofconn, oh);
> > +        OVS_NOT_REACHED();
> 
> Grouping this with the OFPTYPE_TABLE_FEATURES_STATS_REQUEST case and adding a comment that they are handled by the multi-part requests might make it clearer why this should not be reached.
> 
> Acked-by: Justin Pettit <jpettit@ovn.org>

Thanks, I fixed that stuff and applied this to master.
diff mbox series

Patch

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index d65a3fea1559..54f56d9f100e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6344,49 +6344,60 @@  flow_monitor_delete(struct ofconn *ofconn, uint32_t id)
 }
 
 static enum ofperr
-handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
+handle_flow_monitor_request(struct ofconn *ofconn, const struct ovs_list *msgs)
     OVS_EXCLUDED(ofproto_mutex)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
 
-    struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
-
     struct ofmonitor **monitors = NULL;
     size_t allocated_monitors = 0;
     size_t n_monitors = 0;
 
-    enum ofperr error;
-
     ovs_mutex_lock(&ofproto_mutex);
-    for (;;) {
-        struct ofputil_flow_monitor_request request;
-        struct ofmonitor *m;
-        int retval;
+    struct ofpbuf *b;
+    LIST_FOR_EACH (b, list_node, msgs) {
+        for (;;) {
+            enum ofperr error;
 
-        retval = ofputil_decode_flow_monitor_request(&request, &b);
-        if (retval == EOF) {
-            break;
-        } else if (retval) {
-            error = retval;
-            goto error;
-        }
+            struct ofputil_flow_monitor_request request;
+            int retval = ofputil_decode_flow_monitor_request(&request, b);
+            if (retval == EOF) {
+                break;
+            } else if (retval) {
+                error = retval;
+                goto error;
+            }
 
-        if (request.table_id != 0xff
-            && request.table_id >= ofproto->n_tables) {
-            error = OFPERR_OFPBRC_BAD_TABLE_ID;
-            goto error;
-        }
+            if (request.table_id != 0xff
+                && request.table_id >= ofproto->n_tables) {
+                error = OFPERR_OFPBRC_BAD_TABLE_ID;
+                goto error;
+            }
 
-        error = ofmonitor_create(&request, ofconn, &m);
-        if (error) {
-            goto error;
-        }
+            struct ofmonitor *m;
+            error = ofmonitor_create(&request, ofconn, &m);
+            if (error) {
+                goto error;
+            }
 
-        if (n_monitors >= allocated_monitors) {
-            monitors = x2nrealloc(monitors, &allocated_monitors,
-                                  sizeof *monitors);
+            if (n_monitors >= allocated_monitors) {
+                monitors = x2nrealloc(monitors, &allocated_monitors,
+                                      sizeof *monitors);
+            }
+            monitors[n_monitors++] = m;
+            continue;
+
+        error:
+            ofconn_send_error(ofconn, b->data, error);
+
+            for (size_t i = 0; i < n_monitors; i++) {
+                ofmonitor_destroy(monitors[i]);
+            }
+            free(monitors);
+            ovs_mutex_unlock(&ofproto_mutex);
+
+            return error;
         }
-        monitors[n_monitors++] = m;
     }
 
     struct rule_collection rules;
@@ -6396,7 +6407,7 @@  handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
     }
 
     struct ovs_list replies;
-    ofpmp_init(&replies, oh);
+    ofpmp_init(&replies, ofpbuf_from_list(ovs_list_back(msgs))->header);
     ofmonitor_compose_refresh_updates(&rules, &replies);
     ovs_mutex_unlock(&ofproto_mutex);
 
@@ -6406,15 +6417,6 @@  handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
     free(monitors);
 
     return 0;
-
-error:
-    for (size_t i = 0; i < n_monitors; i++) {
-        ofmonitor_destroy(monitors[i]);
-    }
-    free(monitors);
-    ovs_mutex_unlock(&ofproto_mutex);
-
-    return error;
 }
 
 static enum ofperr
@@ -8398,7 +8400,7 @@  handle_single_part_openflow(struct ofconn *ofconn, const struct ofp_header *oh,
         return handle_port_desc_stats_request(ofconn, oh);
 
     case OFPTYPE_FLOW_MONITOR_STATS_REQUEST:
-        return handle_flow_monitor_request(ofconn, oh);
+        OVS_NOT_REACHED();
 
     case OFPTYPE_METER_STATS_REQUEST:
     case OFPTYPE_METER_CONFIG_STATS_REQUEST:
@@ -8496,6 +8498,8 @@  handle_openflow(struct ofconn *ofconn, const struct ovs_list *msgs)
     if (!error) {
         if (type == OFPTYPE_TABLE_FEATURES_STATS_REQUEST) {
             handle_table_features_request(ofconn, msgs);
+        } else if (type == OFPTYPE_FLOW_MONITOR_STATS_REQUEST) {
+            handle_flow_monitor_request(ofconn, msgs);
         } else if (!ovs_list_is_short(msgs)) {
             error = OFPERR_OFPBRC_BAD_STAT;
         } else {