app_queue: Fix deadlock between update and show queues

Operations that update queues when shared_lastcall is set lock the
queue in question, then have to lock the queues container to find the
other queues with the same member. On the other hand, __queues_show
(which is called by both the CLI and AMI) does the reverse. It locks
the queues container, then iterates over the queues locking each in
turn to display them.  This creates a deadlock.

* Moved queue print logic from __queues_show to a separate function
  that can be called for a single queue.

* Updated __queues_show so it doesn't need to lock or traverse
  the queues container to show a single queue.

* Updated __queues_show to snap a copy of the queues container and iterate
  over that instead of locking the queues container and iterating over
  it while locked.  This prevents us from having to hold both the
  container lock and the queue locks at the same time.  This also
  allows us to sort the queue entries.

ASTERISK-29155

Change-Id: I78d4dc36728c2d7bc187b97d82673fc77f2bcf41

diff --git a/apps/app_queue.c b/apps/app_queue.c
index d94a8099aab9aaf59bc33f2fb08e0b3e1bcd1568..db98cea613fcc96ecfb19e73bb2cb59eb9b3899f 100644 (file)
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -9647,6 +9647,105 @@ static void do_print(struct mansession *s, int fd, const char *str)
        }
 }
 
+/*! \brief Print a single queue to AMI or the CLI */
+static void print_queue(struct mansession *s, int fd, struct call_queue *q)
+{
+       float sl;
+       float sl2;
+       struct ao2_iterator mem_iter;
+       struct ast_str *out = ast_str_alloca(512);
+       time_t now = time(NULL);
+
+       ast_str_set(&out, 0, "%s has %d calls (max ", q->name, q->count);
+       if (q->maxlen) {
+               ast_str_append(&out, 0, "%d", q->maxlen);
+       } else {
+               ast_str_append(&out, 0, "unlimited");
+       }
+       sl = 0;
+       sl2 = 0;
+       if (q->callscompleted > 0) {
+               sl = 100 * ((float) q->callscompletedinsl / (float) q->callscompleted);
+       }
+       if (q->callscompleted + q->callsabandoned > 0) {
+               sl2 =100 * (((float)q->callsabandonedinsl + (float)q->callscompletedinsl) / ((float)q->callsabandoned + (float)q->callscompleted));
+       }
+
+       ast_str_append(&out, 0, ") in '%s' strategy (%ds holdtime, %ds talktime), W:%d, C:%d, A:%d, SL:%2.1f%%, SL2:%2.1f%% within %ds",
+               int2strat(q->strategy), q->holdtime, q->talktime, q->weight, q->callscompleted, q->callsabandoned, sl, sl2, q->servicelevel);
+       do_print(s, fd, ast_str_buffer(out));
+       if (!ao2_container_count(q->members)) {
+               do_print(s, fd, "   No Members");
+       } else {
+               struct member *mem;
+
+               do_print(s, fd, "   Members: ");
+               mem_iter = ao2_iterator_init(q->members, 0);
+               while ((mem = ao2_iterator_next(&mem_iter))) {
+                       ast_str_set(&out, 0, "      %s", mem->membername);
+                       if (strcasecmp(mem->membername, mem->interface)) {
+                               ast_str_append(&out, 0, " (%s", mem->interface);
+                               if (!ast_strlen_zero(mem->state_interface)
+                                       && strcmp(mem->state_interface, mem->interface)) {
+                                       ast_str_append(&out, 0, " from %s", mem->state_interface);
+                               }
+                               ast_str_append(&out, 0, ")");
+                       }
+                       if (mem->penalty) {
+                               ast_str_append(&out, 0, " with penalty %d", mem->penalty);
+                       }
+
+                       ast_str_append(&out, 0, " (ringinuse %s)", mem->ringinuse ? "enabled" : "disabled");
+
+                       ast_str_append(&out, 0, "%s%s%s%s%s%s%s%s%s",
+                               mem->dynamic ? ast_term_color(COLOR_CYAN, COLOR_BLACK) : "", mem->dynamic ? " (dynamic)" : "", ast_term_reset(),
+                               mem->realtime ? ast_term_color(COLOR_MAGENTA, COLOR_BLACK) : "", mem->realtime ? " (realtime)" : "", ast_term_reset(),
+                               mem->starttime ? ast_term_color(COLOR_BROWN, COLOR_BLACK) : "", mem->starttime ? " (in call)" : "", ast_term_reset());
+
+                       if (mem->paused) {
+                               ast_str_append(&out, 0, " %s(paused%s%s was %ld secs ago)%s",
+                                       ast_term_color(COLOR_BROWN, COLOR_BLACK),
+                                       ast_strlen_zero(mem->reason_paused) ? "" : ":",
+                                       ast_strlen_zero(mem->reason_paused) ? "" : mem->reason_paused,
+                                       (long) (now - mem->lastpause),
+                                       ast_term_reset());
+                       }
+
+                       ast_str_append(&out, 0, " (%s%s%s)",
+                               ast_term_color(
+                                       mem->status == AST_DEVICE_UNAVAILABLE || mem->status == AST_DEVICE_UNKNOWN ?
+                                               COLOR_RED : COLOR_GREEN, COLOR_BLACK),
+                                       ast_devstate2str(mem->status), ast_term_reset());
+                       if (mem->calls) {
+                               ast_str_append(&out, 0, " has taken %d calls (last was %ld secs ago)",
+                                       mem->calls, (long) (now - mem->lastcall));
+                       } else {
+                               ast_str_append(&out, 0, " has taken no calls yet");
+                       }
+                       do_print(s, fd, ast_str_buffer(out));
+                       ao2_ref(mem, -1);
+               }
+               ao2_iterator_destroy(&mem_iter);
+       }
+       if (!q->head) {
+               do_print(s, fd, "   No Callers");
+       } else {
+               struct queue_ent *qe;
+               int pos = 1;
+
+               do_print(s, fd, "   Callers: ");
+               for (qe = q->head; qe; qe = qe->next) {
+                       ast_str_set(&out, 0, "      %d. %s (wait: %ld:%2.2ld, prio: %d)",
+                               pos++, ast_channel_name(qe->chan), (long) (now - qe->start) / 60,
+                               (long) (now - qe->start) % 60, qe->prio);
+                       do_print(s, fd, ast_str_buffer(out));
+               }
+       }
+       do_print(s, fd, "");    /* blank line between entries */
+}
+
+AO2_STRING_FIELD_SORT_FN(call_queue, name);
+
 /*!
  * \brief Show queue(s) status and statistics
  *
@@ -9657,10 +9756,10 @@ static char *__queues_show(struct mansession *s, int fd, int argc, const char *
 {
        struct call_queue *q;
        struct ast_str *out = ast_str_alloca(512);
-       int found = 0;
-       time_t now = time(NULL);
+       struct ao2_container *sorted_queues;
+
        struct ao2_iterator queue_iter;
-       struct ao2_iterator mem_iter;
+       int found = 0;
 
        if (argc != 2 && argc != 3) {
                return CLI_SHOWUSAGE;
@@ -9668,9 +9767,18 @@ static char *__queues_show(struct mansession *s, int fd, int argc, const char *
 
        if (argc == 3)  { /* specific queue */
                if ((q = find_load_queue_rt_friendly(argv[2]))) {
-                       queue_t_unref(q, "Done with temporary pointer");
+                       ao2_lock(q);
+                       print_queue(s, fd, q);
+                       ao2_unlock(q);
+                       queue_unref(q);
+               } else {
+                       ast_str_set(&out, 0, "No such queue: %s.", argv[2]);
+                       do_print(s, fd, ast_str_buffer(out));
                }
-       } else if (ast_check_realtime("queues")) {
+               return CLI_SUCCESS;
+       }
+
+       if (ast_check_realtime("queues")) {
                /* This block is to find any queues which are defined in realtime but
                 * which have not yet been added to the in-core container
                 */
@@ -9687,21 +9795,34 @@ static char *__queues_show(struct mansession *s, int fd, int argc, const char *
                }
        }
 
-       ao2_lock(queues);
-       queue_iter = ao2_iterator_init(queues, AO2_ITERATOR_DONTLOCK);
-       while ((q = ao2_t_iterator_next(&queue_iter, "Iterate through queues"))) {
-               float sl;
-               float sl2;
+       /*
+        * Snapping a copy of the container prevents having to lock both the queues container
+        * and the queue itself at the same time.  It also allows us to sort the entries.
+        */
+       sorted_queues = ao2_container_alloc_rbtree(AO2_ALLOC_OPT_LOCK_NOLOCK, 0, call_queue_sort_fn, NULL);
+       if (!sorted_queues) {
+               return CLI_SUCCESS;
+       }
+       if (ao2_container_dup(sorted_queues, queues, 0)) {
+               ao2_ref(sorted_queues, -1);
+               return CLI_SUCCESS;
+       }
 
+       /*
+        * No need to lock the container since it's temporary and static.
+        * We also unlink the entries as we use them so the container is
+        * empty when the iterator finishes.  We can then just unref the container.
+        */
+       queue_iter = ao2_iterator_init(sorted_queues, AO2_ITERATOR_DONTLOCK | AO2_ITERATOR_UNLINK);
+       while ((q = ao2_t_iterator_next(&queue_iter, "Iterate through queues"))) {
                struct call_queue *realtime_queue = NULL;
-
                ao2_lock(q);
                /* This check is to make sure we don't print information for realtime
                 * queues which have been deleted from realtime but which have not yet
                 * been deleted from the in-core container. Only do this if we're not
                 * looking for a specific queue.
                 */
-               if (argc < 3 && q->realtime) {
+               if (q->realtime) {
                        realtime_queue = find_load_queue_rt_friendly(q->name);
                        if (!realtime_queue) {
                                ao2_unlock(q);
@@ -9711,110 +9832,16 @@ static char *__queues_show(struct mansession *s, int fd, int argc, const char *
                        queue_t_unref(realtime_queue, "Queue is already in memory");
                }
 
-               if (argc == 3 && strcasecmp(q->name, argv[2])) {
-                       ao2_unlock(q);
-                       queue_t_unref(q, "Done with iterator");
-                       continue;
-               }
                found = 1;
+               print_queue(s, fd, q);
 
-               ast_str_set(&out, 0, "%s has %d calls (max ", q->name, q->count);
-               if (q->maxlen) {
-                       ast_str_append(&out, 0, "%d", q->maxlen);
-               } else {
-                       ast_str_append(&out, 0, "unlimited");
-               }
-               sl = 0;
-               sl2 = 0;
-               if (q->callscompleted > 0) {
-                       sl = 100 * ((float) q->callscompletedinsl / (float) q->callscompleted);
-               }
-               if (q->callscompleted + q->callsabandoned > 0) {
-                       sl2 =100 * (((float)q->callsabandonedinsl + (float)q->callscompletedinsl) / ((float)q->callsabandoned + (float)q->callscompleted));
-               }
-
-               ast_str_append(&out, 0, ") in '%s' strategy (%ds holdtime, %ds talktime), W:%d, C:%d, A:%d, SL:%2.1f%%, SL2:%2.1f%% within %ds",
-                       int2strat(q->strategy), q->holdtime, q->talktime, q->weight, q->callscompleted, q->callsabandoned, sl, sl2, q->servicelevel);
-               do_print(s, fd, ast_str_buffer(out));
-               if (!ao2_container_count(q->members)) {
-                       do_print(s, fd, "   No Members");
-               } else {
-                       struct member *mem;
-
-                       do_print(s, fd, "   Members: ");
-                       mem_iter = ao2_iterator_init(q->members, 0);
-                       while ((mem = ao2_iterator_next(&mem_iter))) {
-                               ast_str_set(&out, 0, "      %s", mem->membername);
-                               if (strcasecmp(mem->membername, mem->interface)) {
-                                       ast_str_append(&out, 0, " (%s", mem->interface);
-                                       if (!ast_strlen_zero(mem->state_interface)
-                                               && strcmp(mem->state_interface, mem->interface)) {
-                                               ast_str_append(&out, 0, " from %s", mem->state_interface);
-                                       }
-                                       ast_str_append(&out, 0, ")");
-                               }
-                               if (mem->penalty) {
-                                       ast_str_append(&out, 0, " with penalty %d", mem->penalty);
-                               }
-
-                               ast_str_append(&out, 0, " (ringinuse %s)", mem->ringinuse ? "enabled" : "disabled");
-
-                               ast_str_append(&out, 0, "%s%s%s%s%s%s%s%s%s",
-                                       mem->dynamic ? ast_term_color(COLOR_CYAN, COLOR_BLACK) : "", mem->dynamic ? " (dynamic)" : "", ast_term_reset(),
-                                       mem->realtime ? ast_term_color(COLOR_MAGENTA, COLOR_BLACK) : "", mem->realtime ? " (realtime)" : "", ast_term_reset(),
-                                       mem->starttime ? ast_term_color(COLOR_BROWN, COLOR_BLACK) : "", mem->starttime ? " (in call)" : "", ast_term_reset());
-
-                               if (mem->paused) {
-                                       ast_str_append(&out, 0, " %s(paused%s%s was %ld secs ago)%s",
-                                               ast_term_color(COLOR_BROWN, COLOR_BLACK),
-                                               ast_strlen_zero(mem->reason_paused) ? "" : ":",
-                                               ast_strlen_zero(mem->reason_paused) ? "" : mem->reason_paused,
-                                               (long) (now - mem->lastpause),
-                                               ast_term_reset());
-                               }
-
-                               ast_str_append(&out, 0, " (%s%s%s)",
-                                       ast_term_color(
-                                               mem->status == AST_DEVICE_UNAVAILABLE || mem->status == AST_DEVICE_UNKNOWN ?
-                                                       COLOR_RED : COLOR_GREEN, COLOR_BLACK),
-                                               ast_devstate2str(mem->status), ast_term_reset());
-                               if (mem->calls) {
-                                       ast_str_append(&out, 0, " has taken %d calls (last was %ld secs ago)",
-                                               mem->calls, (long) (now - mem->lastcall));
-                               } else {
-                                       ast_str_append(&out, 0, " has taken no calls yet");
-                               }
-                               do_print(s, fd, ast_str_buffer(out));
-                               ao2_ref(mem, -1);
-                       }
-                       ao2_iterator_destroy(&mem_iter);
-               }
-               if (!q->head) {
-                       do_print(s, fd, "   No Callers");
-               } else {
-                       struct queue_ent *qe;
-                       int pos = 1;
-
-                       do_print(s, fd, "   Callers: ");
-                       for (qe = q->head; qe; qe = qe->next) {
-                               ast_str_set(&out, 0, "      %d. %s (wait: %ld:%2.2ld, prio: %d)",
-                                       pos++, ast_channel_name(qe->chan), (long) (now - qe->start) / 60,
-                                       (long) (now - qe->start) % 60, qe->prio);
-                               do_print(s, fd, ast_str_buffer(out));
-                       }
-               }
-               do_print(s, fd, "");    /* blank line between entries */
                ao2_unlock(q);
                queue_t_unref(q, "Done with iterator"); /* Unref the iterator's reference */
        }
        ao2_iterator_destroy(&queue_iter);
-       ao2_unlock(queues);
+       ao2_ref(sorted_queues, -1);
        if (!found) {
-               if (argc == 3) {
-                       ast_str_set(&out, 0, "No such queue: %s.", argv[2]);
-               } else {
-                       ast_str_set(&out, 0, "No queues.");
-               }
+               ast_str_set(&out, 0, "No queues.");
                do_print(s, fd, ast_str_buffer(out));
        }
        return CLI_SUCCESS;