[ovs-dev] [meters 9/9] ofproto: Break handle_meter_mod() into sub-functions.

Ben Pfaff blp at nicira.com
Thu Jun 27 22:19:44 UTC 2013


This makes the code more obviously correct, to my eye, especially
regarding the freeing of the 'band' ofpbuf.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/ofproto.c |  192 ++++++++++++++++++++++++++++------------------------
 1 files changed, 103 insertions(+), 89 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 22b5fd4..375f844 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4257,6 +4257,100 @@ meter_create(const struct ofputil_meter_config *config,
 }
 
 static enum ofperr
+handle_add_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm)
+{
+    ofproto_meter_id provider_meter_id = { UINT32_MAX };
+    struct meter **meterp = &ofproto->meters[mm->meter.meter_id];
+    enum ofperr error;
+
+    if (*meterp) {
+        return OFPERR_OFPMMFC_METER_EXISTS;
+    }
+
+    error = ofproto->ofproto_class->meter_set(ofproto, &provider_meter_id,
+                                              &mm->meter);
+    if (!error) {
+        ovs_assert(provider_meter_id.uint32 != UINT32_MAX);
+        *meterp = meter_create(&mm->meter, provider_meter_id);
+    }
+    return 0;
+}
+
+static enum ofperr
+handle_modify_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm)
+{
+    struct meter *meter = ofproto->meters[mm->meter.meter_id];
+    enum ofperr error;
+
+    if (!meter) {
+        return OFPERR_OFPMMFC_UNKNOWN_METER;
+    }
+
+    error = ofproto->ofproto_class->meter_set(ofproto,
+                                              &meter->provider_meter_id,
+                                              &mm->meter);
+    ovs_assert(meter->provider_meter_id.uint32 != UINT32_MAX);
+    if (!error) {
+        meter_update(meter, &mm->meter);
+    }
+    return error;
+}
+
+static enum ofperr
+handle_delete_meter(struct ofconn *ofconn, const struct ofp_header *oh,
+                    struct ofputil_meter_mod *mm)
+{
+    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
+    uint32_t meter_id = mm->meter.meter_id;
+    uint32_t first, last;
+    struct list rules;
+
+    if (meter_id == OFPM13_ALL) {
+        first = 1;
+        last = ofproto->meter_features.max_meters;
+    } else {
+        if (!meter_id || meter_id > ofproto->meter_features.max_meters) {
+            return 0;
+        }
+        first = last = meter_id;
+    }
+
+    /* First delete the rules that use this meter.  If any of those rules are
+     * currently being modified, postpone the whole operation until later. */
+    list_init(&rules);
+    for (meter_id = first; meter_id <= last; ++meter_id) {
+        struct meter *meter = ofproto->meters[meter_id];
+        if (meter && !list_is_empty(&meter->rules)) {
+            struct rule *rule;
+
+            LIST_FOR_EACH (rule, meter_list_node, &meter->rules) {
+                if (rule->pending) {
+                    return OFPROTO_POSTPONE;
+                }
+                list_push_back(&rules, &rule->ofproto_node);
+            }
+        }
+    }
+    if (!list_is_empty(&rules)) {
+        delete_flows__(ofproto, ofconn, oh, &rules, OFPRR_METER_DELETE);
+    }
+
+    /* Delete the meters. */
+    for (meter_id = first; meter_id <= last; ++meter_id) {
+        struct meter *meter = ofproto->meters[meter_id];
+        if (meter) {
+            ofproto->meters[meter_id] = NULL;
+            ofproto->ofproto_class->meter_del(ofproto,
+                                              meter->provider_meter_id);
+            free(meter->bands);
+            free(meter);
+        }
+    }
+
+    return 0;
+}
+
+static enum ofperr
 handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
@@ -4268,7 +4362,7 @@ handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh)
 
     error = reject_slave_controller(ofconn);
     if (error) {
-        goto exit;
+        return error;
     }
 
     ofpbuf_use_stub(&bands, bands_stub, sizeof bands_stub);
@@ -4293,105 +4387,25 @@ handle_meter_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     }
 
     switch (mm.command) {
-    case OFPMC13_ADD: {
-        ofproto_meter_id provider_meter_id = { UINT32_MAX };
-
-        if (ofproto->meters[meter_id]) {
-            error = OFPERR_OFPMMFC_METER_EXISTS;
-            break;
-        }
-
-        error = ofproto->ofproto_class->meter_set(ofproto, &provider_meter_id,
-                                                  &mm.meter);
-        if (!error) {
-            ovs_assert(provider_meter_id.uint32 != UINT32_MAX);
-            ofproto->meters[meter_id] = meter_create(&mm.meter,
-                                                     provider_meter_id);
-        }
+    case OFPMC13_ADD:
+        error = handle_add_meter(ofproto, &mm);
         break;
-    }
-    case OFPMC13_MODIFY: {
-        struct meter *meter = ofproto->meters[meter_id];
 
-        if (!meter) {
-            error = OFPERR_OFPMMFC_UNKNOWN_METER;
-            break;
-        }
-
-        error = ofproto->ofproto_class->meter_set(ofproto,
-                                                  &meter->provider_meter_id,
-                                                  &mm.meter);
-        ovs_assert(meter->provider_meter_id.uint32 != UINT32_MAX);
-        if (!error) {
-            meter_update(meter, &mm.meter);
-        }
+    case OFPMC13_MODIFY:
+        error = handle_modify_meter(ofproto, &mm);
         break;
-    }
-    case OFPMC13_DELETE: {
-        struct list rules;
-        uint32_t first, last;
 
-        if (meter_id == OFPM13_ALL) {
-            first = 1;
-            last = ofproto->meter_features.max_meters;
-        } else {
-            if (!meter_id || meter_id > ofproto->meter_features.max_meters) {
-                return 0;
-            }
-            first = last = meter_id;
-        }
-
-        /* First collect flows to be deleted, possibly postponing
-         * the whole operation. */
-        list_init(&rules);
-        for (meter_id = first; meter_id <= last; ++meter_id) {
-            struct meter *meter = ofproto->meters[meter_id];
-            if (!meter) {
-                continue;
-            }
-
-            if (!list_is_empty(&meter->rules)) {
-                struct rule *rule, *next_rule;
-                /* Move rules depending on this meter to be deleted later. */
-                LIST_FOR_EACH_SAFE (rule, next_rule, meter_list_node,
-                                    &meter->rules) {
-                    if (rule->pending) {
-                        return OFPROTO_POSTPONE;
-                    }
-                    list_push_back(&rules, &rule->ofproto_node);
-                }
-            }
-        }
-
-        /* Delete the collected rules. */
-        if (!list_is_empty(&rules)) {
-            delete_flows__(ofproto, ofconn, oh, &rules, OFPRR_METER_DELETE);
-        }
-
-        /* Delete the meters. */
-        for (meter_id = first; meter_id <= last; ++meter_id) {
-            struct meter *meter = ofproto->meters[meter_id];
-            if (!meter) {
-                continue; /* Skip non-existing meters. */
-            }
-
-            ofproto->meters[meter_id] = NULL;
-            ofproto->ofproto_class->meter_del(ofproto,
-                                              meter->provider_meter_id);
-            free(meter->bands);
-            free(meter);
-        }
+    case OFPMC13_DELETE:
+        error = handle_delete_meter(ofconn, oh, &mm);
+        break;
 
-        /* Delete does not parse bands, no need to free. */
-        return 0;
-    }
     default:
         error = OFPERR_OFPMMFC_BAD_COMMAND;
+        break;
     }
 
 exit_free_bands:
     ofpbuf_uninit(&bands);
-exit:
     return error;
 }
 
-- 
1.7.2.5




More information about the dev mailing list