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

[Ryan Wilson 76511]

This is a much cleaner improvement, thanks!

Acked-by: Ryan Wilson <wryan at nicira.com>

On 5/22/14 11:04 AM, "Andy Zhou" <azhou at nicira.com> wrote:

>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
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbffg%
>3D%3D%0A&m=zh8wMPw9xcbpj4PL38Ny0FxIVZca52l1YMLz2Bd8XTg%3D%0A&s=5cbf10fbd63
>99c57836e3dcdba2ac6c567f16aaba5a2658c52298cc2ecc21e89