astobj2: Avoid using temporary objects + ao2_find() with OBJ_POINTER.

There is a fairly common pattern making its way through the code base where we
put a temporary object on the stack so we can call ao2_find() with OBJ_POINTER.
The purpose is so that it can be passed into the object hash function.
However, this really seems like a hack and potentially error prone.  This patch
is a first stab at approach to avoid having to do that.

It adds a new flag, OBJ_KEY, which can be used instead of OBJ_POINTER in these
situations.  Then, the hash function can know whether it was given an object or
some custom data to hash.

The patch also changes some uses of ao2_find() for iax2_user and iax2_peer
objects to reflect how OBJ_KEY would be used.

So long, and thanks for all the fish.

Review: https://reviewboard.asterisk.org/r/1184/

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@330273 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index dff1bfda21f82b05dcb27b2ce87851d56a2c721a..71844d68ff73477a2d4a42a86266a523a5e5a675 100644 (file)
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -1726,8 +1726,9 @@ static int iax2_data_add_codecs(struct ast_data *root, const char *node_name, ia
 static int peer_hash_cb(const void *obj, const int flags)
 {
        const struct iax2_peer *peer = obj;
+       const char *name = obj;
 
-       return ast_str_hash(peer->name);
+       return ast_str_hash(flags & OBJ_KEY ? name : peer->name);
 }
 
 /*!
@@ -1736,8 +1737,10 @@ static int peer_hash_cb(const void *obj, const int flags)
 static int peer_cmp_cb(void *obj, void *arg, int flags)
 {
        struct iax2_peer *peer = obj, *peer2 = arg;
+       const char *name = arg;
 
-       return !strcmp(peer->name, peer2->name) ? CMP_MATCH | CMP_STOP : 0;
+       return !strcmp(peer->name, flags & OBJ_KEY ? name : peer2->name) ?
+                       CMP_MATCH | CMP_STOP : 0;
 }
 
 /*!
@@ -1746,8 +1749,9 @@ static int peer_cmp_cb(void *obj, void *arg, int flags)
 static int user_hash_cb(const void *obj, const int flags)
 {
        const struct iax2_user *user = obj;
+       const char *name = obj;
 
-       return ast_str_hash(user->name);
+       return ast_str_hash(flags & OBJ_KEY ? name : user->name);
 }
 
 /*!
@@ -1756,8 +1760,10 @@ static int user_hash_cb(const void *obj, const int flags)
 static int user_cmp_cb(void *obj, void *arg, int flags)
 {
        struct iax2_user *user = obj, *user2 = arg;
+       const char *name = arg;
 
-       return !strcmp(user->name, user2->name) ? CMP_MATCH | CMP_STOP : 0;
+       return !strcmp(user->name, flags & OBJ_KEY ? name : user2->name) ?
+                       CMP_MATCH | CMP_STOP : 0;
 }
 
 /*!
@@ -1767,11 +1773,8 @@ static int user_cmp_cb(void *obj, void *arg, int flags)
 static struct iax2_peer *find_peer(const char *name, int realtime) 
 {
        struct iax2_peer *peer = NULL;
-       struct iax2_peer tmp_peer = {
-               .name = name,
-       };
 
-       peer = ao2_find(peers, &tmp_peer, OBJ_POINTER);
+       peer = ao2_find(peers, name, OBJ_KEY);
 
        /* Now go for realtime if applicable */
        if(!peer && realtime)
@@ -1794,11 +1797,7 @@ static inline struct iax2_peer *peer_unref(struct iax2_peer *peer)
 
 static struct iax2_user *find_user(const char *name)
 {
-       struct iax2_user tmp_user = {
-               .name = name,
-       };
-
-       return ao2_find(users, &tmp_user, OBJ_POINTER);
+       return ao2_find(users, name, OBJ_KEY);
 }
 static inline struct iax2_user *user_ref(struct iax2_user *user)
 {
@@ -1854,11 +1853,8 @@ static void iax2_destroy_helper(struct chan_iax2_pvt *pvt)
        /* Decrement AUTHREQ count if needed */
        if (ast_test_flag64(pvt, IAX_MAXAUTHREQ)) {
                struct iax2_user *user;
-               struct iax2_user tmp_user = {
-                       .name = pvt->username,
-               };
 
-               user = ao2_find(users, &tmp_user, OBJ_POINTER);
+               user = ao2_find(users, pvt->username, OBJ_KEY);
                if (user) {
                        ast_atomic_fetchadd_int(&user->curauthreq, -1);
                        user_unref(user);
@@ -6943,12 +6939,9 @@ static char *handle_cli_iax2_unregister(struct ast_cli_entry *e, int cmd, struct
        p = find_peer(a->argv[2], 1);
        if (p) {
                if (p->expire > 0) {
-                       struct iax2_peer tmp_peer = {
-                               .name = a->argv[2],
-                       };
                        struct iax2_peer *peer;
 
-                       peer = ao2_find(peers, &tmp_peer, OBJ_POINTER);
+                       peer = ao2_find(peers, a->argv[2], OBJ_KEY);
                        if (peer) {
                                expire_registry(peer_ref(peer)); /* will release its own reference when done */
                                peer_unref(peer); /* ref from ao2_find() */
@@ -7887,11 +7880,9 @@ static int authenticate_request(int call_num)
 
        /* If an AUTHREQ restriction is in place, make sure we can send an AUTHREQ back */
        if (ast_test_flag64(p, IAX_MAXAUTHREQ)) {
-               struct iax2_user *user, tmp_user = {
-                       .name = p->username,    
-               };
+               struct iax2_user *user;
 
-               user = ao2_find(users, &tmp_user, OBJ_POINTER);
+               user = ao2_find(users, p->username, OBJ_KEY);
                if (user) {
                        if (user->curauthreq == user->maxauthreq)
                                authreq_restrict = 1;
@@ -7937,14 +7928,12 @@ static int authenticate_verify(struct chan_iax2_pvt *p, struct iax_ies *ies)
        char rsasecret[256] = "";
        int res = -1; 
        int x;
-       struct iax2_user *user, tmp_user = {
-               .name = p->username,    
-       };
+       struct iax2_user *user;
 
        if (p->authrej) {
                return res;
        }
-       user = ao2_find(users, &tmp_user, OBJ_POINTER);
+       user = ao2_find(users, p->username, OBJ_KEY);
        if (user) {
                if (ast_test_flag64(p, IAX_MAXAUTHREQ)) {
                        ast_atomic_fetchadd_int(&user->curauthreq, -1);
@@ -12400,12 +12389,9 @@ static struct iax2_peer *build_peer(const char *name, struct ast_variable *v, st
        int maskfound = 0;
        int found = 0;
        int firstpass = 1;
-       struct iax2_peer tmp_peer = {
-               .name = name,
-       };
 
        if (!temponly) {
-               peer = ao2_find(peers, &tmp_peer, OBJ_POINTER);
+               peer = ao2_find(peers, name, OBJ_KEY);
                if (peer && !ast_test_flag64(peer, IAX_DELME))
                        firstpass = 0;
        }
@@ -12697,12 +12683,9 @@ static struct iax2_user *build_user(const char *name, struct ast_variable *v, st
        int oldcurauthreq = 0;
        char *varname = NULL, *varval = NULL;
        struct ast_variable *tmpvar = NULL;
-       struct iax2_user tmp_user = {
-               .name = name,
-       };
 
        if (!temponly) {
-               user = ao2_find(users, &tmp_user, OBJ_POINTER);
+               user = ao2_find(users, name, OBJ_KEY);
                if (user && !ast_test_flag64(user, IAX_DELME))
                        firstpass = 0;
        }

diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h
index bf8dd42baec9602d58086a613abab4b57a1e07aa..4b15b8c425c47ec2a6f0dc2ac031bfc67aa3a2bf 100644 (file)
--- a/include/asterisk/astobj2.h
+++ b/include/asterisk/astobj2.h
@@ -680,6 +680,16 @@ enum search_flags {
         * by another mechanism other that the internal ao2_lock.
         */
        OBJ_NOLOCK     = (1 << 5),
+       /*!
+        * \brief The data is hashable, but is not an object.
+        *
+        * This can be used when you want to be able to pass custom data
+        * to a hash function that is not a full object, but perhaps just
+        * a string.
+        *
+        * \note OBJ_KEY and OBJ_POINTER are mutually exclusive options.
+        */
+       OBJ_KEY        = (1 << 6),
 };
 
 /*!
@@ -964,9 +974,9 @@ void *__ao2_callback_data(struct ao2_container *c, enum search_flags flags,
 
 #endif
 
-void *__ao2_find_debug(struct ao2_container *c, void *arg, enum search_flags flags, char *tag,
+void *__ao2_find_debug(struct ao2_container *c, const void *arg, enum search_flags flags, char *tag,
                       char *file, int line, const char *funcname);
-void *__ao2_find(struct ao2_container *c, void *arg, enum search_flags flags);
+void *__ao2_find(struct ao2_container *c, const void *arg, enum search_flags flags);
 
 /*! \brief
  *

diff --git a/main/astobj2.c b/main/astobj2.c
index 5e61d5e5f1e260df736b1eeab848d81bff8911e7..45d12a46dc792825a5946f26fe84060e82e5db15 100644 (file)
--- a/main/astobj2.c
+++ b/main/astobj2.c
@@ -648,8 +648,8 @@ static void *internal_ao2_callback(struct ao2_container *c,
         * run the hash function. Otherwise, scan the whole container
         * (this only for the time being. We need to optimize this.)
         */
-       if ((flags & OBJ_POINTER))      /* we know hash can handle this case */
-               start = i = c->hash_fn(arg, flags & OBJ_POINTER) % c->n_buckets;
+       if ((flags & (OBJ_POINTER | OBJ_KEY)))  /* we know hash can handle this case */
+               start = i = c->hash_fn(arg, flags & (OBJ_POINTER | OBJ_KEY)) % c->n_buckets;
        else                    /* don't know, let's scan all buckets */
                start = i = -1;         /* XXX this must be fixed later. */
 
@@ -801,14 +801,14 @@ void *__ao2_callback_data(struct ao2_container *c, const enum search_flags flags
 /*!
  * the find function just invokes the default callback with some reasonable flags.
  */
-void *__ao2_find_debug(struct ao2_container *c, void *arg, enum search_flags flags, char *tag, char *file, int line, const char *funcname)
+void *__ao2_find_debug(struct ao2_container *c, const void *arg, enum search_flags flags, char *tag, char *file, int line, const char *funcname)
 {
-       return __ao2_callback_debug(c, flags, c->cmp_fn, arg, tag, file, line, funcname);
+       return __ao2_callback_debug(c, flags, c->cmp_fn, (void *) arg, tag, file, line, funcname);
 }
 
-void *__ao2_find(struct ao2_container *c, void *arg, enum search_flags flags)
+void *__ao2_find(struct ao2_container *c, const void *arg, enum search_flags flags)
 {
-       return __ao2_callback(c, flags, c->cmp_fn, arg);
+       return __ao2_callback(c, flags, c->cmp_fn, (void *) arg);
 }
 
 /*!

diff --git a/tests/test_astobj2.c b/tests/test_astobj2.c
index 9466f3384ccd7a965da22ef953ccae600713b068..b9e67b5c1349103184650a890c229740c845c7e5 100644 (file)
--- a/tests/test_astobj2.c
+++ b/tests/test_astobj2.c
@@ -38,7 +38,6 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include "asterisk/astobj2.h"
 
 struct test_obj {
-       char c[20];
        int i;
        int *destructor_count;
 };
@@ -75,20 +74,35 @@ static int multiple_cb(void *obj, void *arg, int flag)
 static int test_cmp_cb(void *obj, void *arg, int flags)
 {
        struct test_obj *cmp_obj = (struct test_obj *) obj;
-       struct test_obj *test_obj = (struct test_obj *) arg;
+
        if (!arg) {
                return 0;
        }
-       return (cmp_obj->i == test_obj->i) ? CMP_MATCH | CMP_STOP : 0;
+
+       if (flags & OBJ_KEY) {
+               int *i = (int *) arg;
+               return (cmp_obj->i == *i) ? CMP_MATCH | CMP_STOP : 0;
+       } else {
+               struct test_obj *test_obj = (struct test_obj *) arg;
+               return (cmp_obj->i == test_obj->i) ? CMP_MATCH | CMP_STOP : 0;
+       }
 }
 
 static int test_hash_cb(const void *obj, const int flags)
 {
-       struct test_obj *test_obj = (struct test_obj *) obj;
-       if (!test_obj || ast_strlen_zero(test_obj->c)) {
+       if (!obj) {
                return 0;
        }
-       return ast_str_hash(test_obj->c);
+
+       if (flags & OBJ_KEY) {
+               const int *i = obj;
+
+               return *i;
+       } else {
+               const struct test_obj *test_obj = obj;
+
+               return test_obj->i;
+       }
 }
 
 static int astobj2_test_helper(int use_hash, int use_cmp, unsigned int lim, struct ast_test *test)
@@ -128,7 +142,6 @@ static int astobj2_test_helper(int use_hash, int use_cmp, unsigned int lim, stru
                        res = AST_TEST_FAIL;
                        goto cleanup;
                }
-               snprintf(obj->c, sizeof(obj->c), "zombie #%d", num);
                obj->destructor_count = &destructor_count;
                obj->i = num;
                ao2_link(c1, obj);
@@ -163,7 +176,6 @@ static int astobj2_test_helper(int use_hash, int use_cmp, unsigned int lim, stru
        num = 75;
        for (; num; num--) {
                int i = (ast_random() % ((lim / 2)) + 1); /* find a random object */
-               snprintf(tmp_obj.c, sizeof(tmp_obj.c), "zombie #%d", i);
                tmp_obj.i = i;
                if (!(obj = ao2_find(c1, &tmp_obj, OBJ_POINTER))) {
                        res = AST_TEST_FAIL;
@@ -178,6 +190,23 @@ static int astobj2_test_helper(int use_hash, int use_cmp, unsigned int lim, stru
                }
        }
 
+       /* Testing ao2_find with OBJ_KEY */
+       num = 75;
+       for (; num; num--) {
+               int i = (ast_random() % ((lim / 2)) + 1); /* find a random object */
+               if (!(obj = ao2_find(c1, &i, OBJ_KEY))) {
+                       res = AST_TEST_FAIL;
+                       ast_test_status_update(test, "COULD NOT FIND:%d, ao2_find() with OBJ_KEY flag failed.\n", i);
+               } else {
+                       /* a correct match will only take place when the custom cmp function is used */
+                       if (use_cmp && obj->i != i) {
+                               ast_test_status_update(test, "object %d does not match object %d\n", obj->i, tmp_obj.i);
+                               res = AST_TEST_FAIL;
+                       }
+                       ao2_t_ref(obj, -1, "test");
+               }
+       }
+
        /* Testing ao2_find with OBJ_POINTER | OBJ_UNLINK | OBJ_CONTINUE.
         * In this test items are unlinked from c1 and placed in c2.  Then
         * unlinked from c2 and placed back into c1.
@@ -374,7 +403,7 @@ AST_TEST_DEFINE(astobj2_test_2)
        int num;
        static const int NUM_OBJS = 5;
        int destructor_count = NUM_OBJS;
-       struct test_obj tmp_obj = { "", };
+       struct test_obj tmp_obj = { 0, };
 
        switch (cmd) {
        case TEST_INIT: