This makes the code more obviously correct, to my eye, especially
regarding the freeing of the 'band' ofpbuf.
Signed-off-by: Ben Pfaff <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
