On Mon, Jun 23, 2014 at 05:14:52PM -0700, Ben Pfaff wrote:
> On Mon, Jun 16, 2014 at 11:29:30AM +0900, Simon Horman wrote:
> > Handle modify and delete commands in OpenFlow1.4 flow monitor requests.
> > These commands are not yet allowed by the decoder which
> > will be updated by a subsequent patch.
> > 
> > Signed-off-by: Simon Horman <ho...@verge.net.au>
> > 
> > ---
> > v2
> > * No change
> > ---
> >  ofproto/ofproto.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 3153e71..211f45e 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -4880,12 +4880,23 @@ handle_flow_monitor_request(struct ofconn *ofconn, 
> > const struct ofp_header *oh)
> >              goto error;
> >          }
> >  
> > -        error = ofmonitor_create(&request, ofconn, &m);
> > -        if (error) {
> > -            goto error;
> > +        if (request.command == OFPFMC14_DELETE ||
> > +            request.command == OFPFMC14_MODIFY) {
> > +            error = flow_monitor_delete(ofconn, request.id);
> > +            if (error) {
> > +                goto error;
> > +            }
> >          }
> >  
> > -        list_insert(&monitor_list, &m->list_node);
> > +        if (request.command == OFPFMC14_ADD ||
> > +            request.command == OFPFMC14_MODIFY) {
> > +            error = ofmonitor_create(&request, ofconn, &m);
> > +            if (error) {
> > +                goto error;
> > +            }
> > +
> > +            list_insert(&monitor_list, &m->list_node);
> > +        }
> >      }
> >  
> >      rule_collection_init(&rules);
> 
> I think that this is correct, but this way it is written makes it look
> like a "modify" could fail in the middle with the monitor deleted and
> not re-created.  That can't actually happen because ofmonitor_create()
> only fails if there's a duplicate id.  I'd rather have to code written
> to be more obviously correct.  (One way to do that would be to have an
> ofmonitor_modify() function, I guess, but there might be other ways
> too.)

Thanks, I understand and agree that is good to make things obvious.

I've taken a stab at implementing ofmonitor_modify() in the incremental
change below. Its a bit noisy because of shuffling some existing
infrastructure around. But would something like this address your concern?

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 8891a06..6f8eea3 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -2012,19 +2012,27 @@ static uint64_t monitor_seqno = 1;
 COVERAGE_DEFINE(ofmonitor_pause);
 COVERAGE_DEFINE(ofmonitor_resume);
 
-enum ofperr
-ofmonitor_create(const struct ofputil_flow_monitor_request *request,
-                 struct ofconn *ofconn, struct ofmonitor **monitorp)
+static struct ofmonitor *
+ofmonitor_lookup(struct ofconn *ofconn, uint32_t id)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct ofmonitor *m;
 
-    *monitorp = NULL;
-
-    m = ofmonitor_lookup(ofconn, request->id);
-    if (m) {
-        return OFPERR_OFPMOFC_MONITOR_EXISTS;
+    HMAP_FOR_EACH_IN_BUCKET (m, ofconn_node, hash_int(id, 0),
+                             &ofconn->monitors) {
+        if (m->id == id) {
+            return m;
+        }
     }
+    return NULL;
+}
+
+static struct ofmonitor *
+ofmonitor_create__(const struct ofputil_flow_monitor_request *request,
+                   struct ofconn *ofconn)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct ofmonitor *m;
 
     m = xmalloc(sizeof *m);
     m->ofconn = ofconn;
@@ -2037,23 +2045,59 @@ ofmonitor_create(const struct 
ofputil_flow_monitor_request *request,
     minimatch_init(&m->match, &request->match);
     list_init(&m->list_node);
 
-    *monitorp = m;
+    return m;
+}
+
+enum ofperr
+ofmonitor_create(const struct ofputil_flow_monitor_request *request,
+                 struct ofconn *ofconn, struct ofmonitor **monitorp)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct ofmonitor *m;
+
+    *monitorp = NULL;
+
+    m = ofmonitor_lookup(ofconn, request->id);
+    if (m) {
+        return OFPERR_OFPMOFC_MONITOR_EXISTS;
+    }
+
+    *monitorp = ofmonitor_create__(request, ofconn);
     return 0;
 }
 
-struct ofmonitor *
-ofmonitor_lookup(struct ofconn *ofconn, uint32_t id)
+enum ofperr
+ofmonitor_modify(const struct ofputil_flow_monitor_request *request,
+                 struct ofconn *ofconn, struct ofmonitor **monitorp)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    enum ofperr error;
+
+    error = ofmonitor_delete(request->id, ofconn);
+    if (error) {
+        return error;
+    }
+
+    *monitorp = ofmonitor_create__(request, ofconn);
+    return 0;
+}
+
+enum ofperr
+ofmonitor_delete(uint32_t id, struct ofconn *ofconn)
     OVS_REQUIRES(ofproto_mutex)
 {
     struct ofmonitor *m;
+    enum ofperr error;
 
-    HMAP_FOR_EACH_IN_BUCKET (m, ofconn_node, hash_int(id, 0),
-                             &ofconn->monitors) {
-        if (m->id == id) {
-            return m;
-        }
+    m = ofmonitor_lookup(ofconn, id);
+    if (m) {
+        ofmonitor_destroy(m);
+        error = 0;
+    } else {
+        error = OFPERR_OFPMOFC_UNKNOWN_MONITOR;
     }
-    return NULL;
+
+    return error;
 }
 
 void
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index df024e6..453163b 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -213,10 +213,13 @@ struct ofmonitor {
 
 struct ofputil_flow_monitor_request;
 
-enum ofperr ofmonitor_create(const struct ofputil_flow_monitor_request *,
+enum ofperr ofmonitor_create(const struct ofputil_flow_monitor_request 
*request,
                              struct ofconn *, struct ofmonitor **)
     OVS_REQUIRES(ofproto_mutex);
-struct ofmonitor *ofmonitor_lookup(struct ofconn *, uint32_t id)
+enum ofperr ofmonitor_modify(const struct ofputil_flow_monitor_request 
*request,
+                             struct ofconn *, struct ofmonitor **)
+    OVS_REQUIRES(ofproto_mutex);
+enum ofperr ofmonitor_delete(uint32_t id, struct ofconn *)
     OVS_REQUIRES(ofproto_mutex);
 void ofmonitor_destroy(struct ofmonitor *)
     OVS_REQUIRES(ofproto_mutex);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b90abb5..8121b32 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4915,24 +4915,6 @@ ofmonitor_collect_resume_rules(struct ofmonitor *m,
 }
 
 static enum ofperr
-flow_monitor_delete(struct ofconn *ofconn, uint32_t id)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    struct ofmonitor *m;
-    enum ofperr error;
-
-    m = ofmonitor_lookup(ofconn, id);
-    if (m) {
-        ofmonitor_destroy(m);
-        error = 0;
-    } else {
-        error = OFPERR_OFPMOFC_UNKNOWN_MONITOR;
-    }
-
-    return error;
-}
-
-static enum ofperr
 handle_flow_monitor_request(struct ofconn *ofconn, const struct ofp_header *oh)
     OVS_EXCLUDED(ofproto_mutex)
 {
@@ -4966,21 +4948,29 @@ handle_flow_monitor_request(struct ofconn *ofconn, 
const struct ofp_header *oh)
             goto error;
         }
 
-        if (request.command == OFPFMC14_DELETE ||
-            request.command == OFPFMC14_MODIFY) {
-            error = flow_monitor_delete(ofconn, request.id);
-            if (error) {
-                goto error;
-            }
+        m = NULL;
+        switch (request.command) {
+        case OFPFMC14_ADD:
+            error = ofmonitor_create(&request, ofconn, &m);
+            break;
+
+        case OFPFMC14_MODIFY:
+            error = ofmonitor_modify(&request, ofconn, &m);
+            break;
+
+        case OFPFMC14_DELETE:
+            error = ofmonitor_delete(request.id, ofconn);
+            break;
+
+        default:
+            OVS_NOT_REACHED();
         }
 
-        if (request.command == OFPFMC14_ADD ||
-            request.command == OFPFMC14_MODIFY) {
-            error = ofmonitor_create(&request, ofconn, &m);
-            if (error) {
-                goto error;
-            }
+        if (error) {
+            goto error;
+        }
 
+        if (m) {
             list_push_back(&monitor_list, &m->list_node);
         }
     }
@@ -5022,7 +5012,7 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, const 
struct ofp_header *oh)
     id = ofputil_decode_flow_monitor_cancel(oh);
 
     ovs_mutex_lock(&ofproto_mutex);
-    error = flow_monitor_delete(ofconn, id);
+    error = ofmonitor_delete(id, ofconn);
     ovs_mutex_unlock(&ofproto_mutex);
 
     return error;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to