[ovs-dev] [PATCH 1/4] ofproto: Remove ofproto_group_write_lookup()

Andy Zhou azhou at nicira.com
Thu May 22 18:04:38 UTC 2014


ofproto_group_write_lookup() slightly different from
ofproto_group_lookup() in handling reference counting.
Currently, it has only one caller: modify_group().
It seems the abstraction is not adding value here.

Remove the function, along with some refactoring, makes modify_group()
easier to understand.

Signed-off-by: Andy Zhou <azhou at nicira.com>
---
 ofproto/ofproto.c | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7952984..7086960 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5352,40 +5352,38 @@ handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request,
     return 0;
 }
 
-/* If the group exists, this function increments the groups's reference count.
- *
- * Make sure to call ofproto_group_unref() after no longer needing to maintain
- * a reference to the group. */
-bool
-ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
-                     struct ofgroup **group)
+static bool
+ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id,
+                       struct ofgroup **group)
+    OVS_REQ_RDLOCK(ofproto->groups_rwlock)
 {
-    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
     HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node,
                              hash_int(group_id, 0), &ofproto->groups) {
         if ((*group)->group_id == group_id) {
-            ofproto_group_ref(*group);
-            ovs_rwlock_unlock(&ofproto->groups_rwlock);
             return true;
         }
     }
-    ovs_rwlock_unlock(&ofproto->groups_rwlock);
+
     return false;
 }
 
-static bool
-ofproto_group_write_lookup(const struct ofproto *ofproto, uint32_t group_id,
-                           struct ofgroup **group)
-    OVS_ACQUIRES(ofproto->groups_rwlock)
+/* If the group exists, this function increments the groups's reference count.
+ *
+ * Make sure to call ofproto_group_unref() after no longer needing to maintain
+ * a reference to the group. */
+bool
+ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
+                     struct ofgroup **group)
 {
-    ovs_rwlock_wrlock(&ofproto->groups_rwlock);
-    HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node,
-                             hash_int(group_id, 0), &ofproto->groups) {
-        if ((*group)->group_id == group_id) {
-            return true;
-        }
+    bool found;
+
+    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
+    found = ofproto_group_lookup__(ofproto, group_id, group);
+    if (found) {
+        ofproto_group_ref(*group);
     }
-    return false;
+    ovs_rwlock_unlock(&ofproto->groups_rwlock);
+    return found;
 }
 
 static bool
@@ -5712,7 +5710,8 @@ modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
 
     retiring = new_ofgroup;
 
-    if (!ofproto_group_write_lookup(ofproto, gm->group_id, &ofgroup)) {
+    ovs_rwlock_wrlock(&ofproto->groups_rwlock);
+    if (!ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup)) {
         error = OFPERR_OFPGMFC_UNKNOWN_GROUP;
         goto out;
     }
-- 
1.9.1




More information about the dev mailing list