Merged revisions 336094 via svnmerge from

https://origsvn.digium.com/svn/asterisk/branches/10

................
  r336094 | irroot | 2011-09-15 17:54:46 +0200 (Thu, 15 Sep 2011) | 26 lines

  Merged revisions 336093 via svnmerge from
  https://origsvn.digium.com/svn/asterisk/branches/1.8

  ........
    r336093 | irroot | 2011-09-15 17:46:21 +0200 (Thu, 15 Sep 2011) | 20 lines

    Locking order in app_queue.c causes deadlocks.

    a channel lock must never be held with the queues container lock held.

    the deadlock occured on masquerade.

    the queues container lock is a relic of the past the old queue module lock.
    with ao2 there is no need to hold this lock when dealing with members this
    patch removes unneeded locks.

    (closes issue ASTERISK-18101)
    (closes issue ASTERISK-18487)
    Reported by: Paul Rolfe, Jason Legault
    Tested by: irroot, Jason Legault, Paul Rolfe
    Reviewed by: Matthew Nicholson

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

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

diff --git a/apps/app_queue.c b/apps/app_queue.c
index 07b8d7d..7d8493a 100644 (file)
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -2418,13 +2418,11 @@ static struct call_queue *load_realtime_queue(const char *queuename)
                        queue_t_unref(q, "Need to find realtime queue");
                }
 
-               ao2_lock(queues);
-
                q = find_queue_by_name_rt(queuename, queue_vars, member_config);
                ast_config_destroy(member_config);
                ast_variables_destroy(queue_vars);
 
-               /* update the use_weight value if the queue's has gained or lost a weight */ 
+               /* update the use_weight value if the queue's has gained or lost a weight */
                if (q) {
                        if (!q->weight && prev_weight) {
                                ast_atomic_fetchadd_int(&use_weight, -1);
@@ -2434,8 +2432,6 @@ static struct call_queue *load_realtime_queue(const char *queuename)
                        }
                }
                /* Other cases will end up with the proper value for use_weight */
-               ao2_unlock(queues);
-
        } else {
                update_realtime_members(q);
        }
@@ -2469,10 +2465,9 @@ static void update_realtime_members(struct call_queue *q)
                return;
        }
 
-       ao2_lock(queues);
        ao2_lock(q);
-       
-       /* Temporarily set realtime  members dead so we can detect deleted ones.*/ 
+
+       /* Temporarily set realtime  members dead so we can detect deleted ones.*/
        mem_iter = ao2_iterator_init(q->members, 0);
        while ((m = ao2_iterator_next(&mem_iter))) {
                if (m->realtime)
@@ -2501,7 +2496,6 @@ static void update_realtime_members(struct call_queue *q)
        }
        ao2_iterator_destroy(&mem_iter);
        ao2_unlock(q);
-       ao2_unlock(queues);
        ast_config_destroy(member_config);
 }
 
@@ -2516,7 +2510,6 @@ static int join_queue(char *queuename, struct queue_ent *qe, enum queue_result *
        if (!(q = load_realtime_queue(queuename)))
                return res;
 
-       ao2_lock(queues);
        ao2_lock(q);
 
        /* This is our one */
@@ -2525,7 +2518,6 @@ static int join_queue(char *queuename, struct queue_ent *qe, enum queue_result *
                if ((status = get_member_status(q, qe->max_penalty, qe->min_penalty, q->joinempty))) {
                        *reason = QUEUE_JOINEMPTY;
                        ao2_unlock(q);
-                       ao2_unlock(queues);
                        queue_t_unref(q, "Done with realtime queue");
                        return res;
                }
@@ -2589,7 +2581,6 @@ static int join_queue(char *queuename, struct queue_ent *qe, enum queue_result *
                ast_debug(1, "Queue '%s' Join, Channel '%s', Position '%d'\n", q->name, qe->chan->name, qe->pos );
        }
        ao2_unlock(q);
-       ao2_unlock(queues);
        queue_t_unref(q, "Done with realtime queue");
 
        return res;
@@ -2982,9 +2973,7 @@ static int compare_weight(struct call_queue *rq, struct member *member)
        struct member *mem;
        int found = 0;
        struct ao2_iterator queue_iter;
-       
-       /* q's lock and rq's lock already set by try_calling()
-        * to solve deadlock */
+
        queue_iter = ao2_iterator_init(queues, 0);
        while ((q = ao2_t_iterator_next(&queue_iter, "Iterate through queues"))) {
                if (q == rq) { /* don't check myself, could deadlock */
@@ -4448,7 +4437,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
        struct ao2_iterator memi;
        struct ast_datastore *datastore, *transfer_ds;
        struct queue_end_bridge *queue_end_bridge = NULL;
-       const int need_weight = use_weight;
 
        ast_channel_lock(qe->chan);
        datastore = ast_channel_datastore_find(qe->chan, &dialed_interface_info, NULL);
@@ -4531,9 +4519,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
                qe->cancel_answered_elsewhere = 1;
        }
 
-       /* Hold the lock while we setup the outgoing calls */
-       if (need_weight)
-               ao2_lock(queues);
        ao2_lock(qe->parent);
        ast_debug(1, "%s is trying to call a queue member.\n",
                                                        qe->chan->name);
@@ -4552,8 +4537,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
                        ao2_ref(cur, -1);
                        ao2_unlock(qe->parent);
                        ao2_iterator_destroy(&memi);
-                       if (need_weight)
-                               ao2_unlock(queues);
                        goto out;
                }
                if (!datastore) {
@@ -4561,8 +4544,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
                                ao2_ref(cur, -1);
                                ao2_unlock(qe->parent);
                                ao2_iterator_destroy(&memi);
-                               if (need_weight)
-                                       ao2_unlock(queues);
                                callattempt_free(tmp);
                                goto out;
                        }
@@ -4571,8 +4552,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
                                ao2_ref(cur, -1);
                                ao2_unlock(&qe->parent);
                                ao2_iterator_destroy(&memi);
-                               if (need_weight)
-                                       ao2_unlock(queues);
                                callattempt_free(tmp);
                                goto out;
                        }
@@ -4609,8 +4588,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
                                ao2_ref(cur, -1);
                                ao2_unlock(qe->parent);
                                ao2_iterator_destroy(&memi);
-                               if (need_weight)
-                                       ao2_unlock(queues);
                                callattempt_free(tmp);
                                goto out;
                        }
@@ -4673,8 +4650,6 @@ static int try_calling(struct queue_ent *qe, const char *options, char *announce
        ++qe->pending;
        ao2_unlock(qe->parent);
        ring_one(qe, outgoing, &numbusies);
-       if (need_weight)
-               ao2_unlock(queues);
        lpeer = wait_for_answer(qe, outgoing, &to, &digit, numbusies, ast_test_flag(&(bridge_config.features_caller), AST_FEATURE_DISCONNECT), forwardsallowed, update_connectedline);
        /* The ast_channel_datastore_remove() function could fail here if the
         * datastore was moved to another channel during a masquerade. If this is
@@ -5279,7 +5254,6 @@ static int remove_from_queue(const char *queuename, const char *interface)
 
        ast_copy_string(tmpmem.interface, interface, sizeof(tmpmem.interface));
        if ((q = ao2_t_find(queues, &tmpq, OBJ_POINTER, "Temporary reference for interface removal"))) {
-               ao2_lock(queues);
                ao2_lock(q);
                if ((mem = ao2_find(q->members, &tmpmem, OBJ_POINTER))) {
                        /* XXX future changes should beware of this assumption!! */
@@ -5290,7 +5264,6 @@ static int remove_from_queue(const char *queuename, const char *interface)
                                ao2_ref(mem, -1);
                                ao2_unlock(q);
                                queue_t_unref(q, "Interface wasn't dynamic, expiring temporary reference");
-                               ao2_unlock(queues);
                                return RES_NOT_DYNAMIC;
                        }
                        q->membercount--;
@@ -5310,7 +5283,6 @@ static int remove_from_queue(const char *queuename, const char *interface)
                        res = RES_EXISTS;
                }
                ao2_unlock(q);
-               ao2_unlock(queues);
                queue_t_unref(q, "Expiring temporary reference");
        }
 
@@ -5335,8 +5307,6 @@ static int add_to_queue(const char *queuename, const char *interface, const char
        if (!(q = load_realtime_queue(queuename)))
                return res;
 
-       ao2_lock(queues);
-
        ao2_lock(q);
        if ((old_member = interface_exists(q, interface)) == NULL) {
                if ((new_member = create_queue_member(interface, membername, penalty, paused, state_interface))) {
@@ -5374,7 +5344,6 @@ static int add_to_queue(const char *queuename, const char *interface, const char
                res = RES_EXISTS;
        }
        ao2_unlock(q);
-       ao2_unlock(queues);
        queue_t_unref(q, "Expiring temporary reference");
 
        return res;
@@ -5554,8 +5523,6 @@ static void reload_queue_members(void)
        struct call_queue *cur_queue;
        char queue_data[PM_MAX_LEN];
 
-       ao2_lock(queues);
-
        /* Each key in 'pm_family' is the name of a queue */
        db_tree = ast_db_gettree(pm_family, NULL);
        for (entry = db_tree; entry; entry = entry->next) {
@@ -5626,7 +5593,6 @@ static void reload_queue_members(void)
                queue_t_unref(cur_queue, "Expire reload reference");
        }
 
-       ao2_unlock(queues);
        if (db_tree) {
                ast_log(LOG_NOTICE, "Queue members successfully reloaded from database.\n");
                ast_db_freetree(db_tree);
@@ -7012,14 +6978,11 @@ static int reload_queues(int reload, struct ast_flags *mask, const char *queuena
                return -1;
        }
 
-       /* We've made it here, so it looks like we're doing operations on all queues. */
-       ao2_lock(queues);
-       
        /* Mark all queues as dead for the moment if we're reloading queues.
         * For clarity, we could just be reloading members, in which case we don't want to mess
         * with the other queue parameters at all*/
        if (queue_reload) {
-               ao2_callback(queues, OBJ_NODATA, mark_dead_and_unfound, (char *) queuename);
+               ao2_callback(queues, OBJ_NODATA | OBJ_NOLOCK, mark_dead_and_unfound, (char *) queuename);
        }
 
        /* Chug through config file */
@@ -7036,17 +6999,16 @@ static int reload_queues(int reload, struct ast_flags *mask, const char *queuena
        ast_config_destroy(cfg);
        /* Unref all the dead queues if we were reloading queues */
        if (queue_reload) {
-               ao2_callback(queues, OBJ_NODATA | OBJ_MULTIPLE | OBJ_UNLINK, kill_dead_queues, (char *) queuename);
+               ao2_callback(queues, OBJ_NODATA | OBJ_MULTIPLE | OBJ_UNLINK | OBJ_NOLOCK, kill_dead_queues, (char *) queuename);
        }
-       ao2_unlock(queues);
        return 0;
 }
-  
+
 /*! \brief Facilitates resetting statistics for a queue
  *
  * This function actually does not reset any statistics, but
  * rather finds a call_queue struct which corresponds to the
- * passed-in queue name and passes that structure to the 
+ * passed-in queue name and passes that structure to the
  * clear_queue function. If no queuename is passed in, then
  * all queues will have their statistics reset.
  *