<p> Attention is currently required from: Jaco Kroon. </p>
<p>Mark Petersen would like Jaco Kroon to <strong>review</strong> this change.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15308">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">contrib/systemd: Added note on common issues with systemd and asterisk<br><br>With newer version of linux /var/run/ is a symlink to /run/ that has<br>been turned into tmpfs.<br><br>Added note that if asterisk has to bind to a specific IP that<br>systemd has to wait until the network is up.<br><br>Added note on how to make sure that the environment variable<br>HOSTNAME is included.<br><br>ASTERISK-29216<br>Reported by: Mark Petersen<br>Tested by: Mark Petersen<br><br>Change-Id: Ib3e560655befd3e99eec743687144f5569533379<br>---<br>M contrib/systemd/asterisk.service<br>M funcs/func_lock.c<br>2 files changed, 65 insertions(+), 109 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/08/15308/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/contrib/systemd/asterisk.service b/contrib/systemd/asterisk.service</span><br><span>index c3d4648..e654f50 100644</span><br><span>--- a/contrib/systemd/asterisk.service</span><br><span>+++ b/contrib/systemd/asterisk.service</span><br><span>@@ -1,15 +1,22 @@</span><br><span> [Unit]</span><br><span> Description=Asterisk PBX and telephony daemon.</span><br><span> After=network.target</span><br><span style="color: hsl(120, 100%, 40%);">+#include these if asterisk need to bind to a specific IP (other than 0.0.0.0)</span><br><span style="color: hsl(120, 100%, 40%);">+#Wants=network-online.target</span><br><span style="color: hsl(120, 100%, 40%);">+#After=network-online.target network.target</span><br><span> </span><br><span> [Service]</span><br><span> Type=notify</span><br><span> Environment=HOME=/var/lib/asterisk</span><br><span style="color: hsl(120, 100%, 40%);">+#if systemd do not provide hostname and you need to use ${ENV(HOSTNAME)}</span><br><span style="color: hsl(120, 100%, 40%);">+#Environment=HOSTNAME=%H</span><br><span> WorkingDirectory=/var/lib/asterisk</span><br><span> User=asterisk</span><br><span> Group=asterisk</span><br><span> ExecStart=/usr/sbin/asterisk -mqf -C /etc/asterisk/asterisk.conf</span><br><span> ExecReload=/usr/sbin/asterisk -rx 'core reload'</span><br><span style="color: hsl(120, 100%, 40%);">+#if /var/run is a tmpfs, this will create /var/run/asterisk on start</span><br><span style="color: hsl(120, 100%, 40%);">+#RuntimeDirectory=asterisk</span><br><span> </span><br><span> #Nice=0</span><br><span> #UMask=0002</span><br><span>diff --git a/funcs/func_lock.c b/funcs/func_lock.c</span><br><span>index acb5fc9..0726407 100644</span><br><span>--- a/funcs/func_lock.c</span><br><span>+++ b/funcs/func_lock.c</span><br><span>@@ -110,7 +110,6 @@</span><br><span> static void lock_free(void *data);</span><br><span> static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_channel *newchan);</span><br><span> static int unloading = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-static pthread_t broker_tid = AST_PTHREADT_NULL;</span><br><span> </span><br><span> static const struct ast_datastore_info lock_info = {</span><br><span> .type = "MUTEX",</span><br><span>@@ -124,8 +123,8 @@</span><br><span> ast_cond_t cond;</span><br><span> /*! count is needed so if a recursive mutex exits early, we know how many times to unlock it. */</span><br><span> unsigned int count;</span><br><span style="color: hsl(0, 100%, 40%);">- /*! Container of requesters for the named lock */</span><br><span style="color: hsl(0, 100%, 40%);">- struct ao2_container *requesters;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*! Count of waiting of requesters for the named lock */</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int requesters;</span><br><span> /*! who owns us */</span><br><span> struct ast_channel *owner;</span><br><span> /*! name of the lock */</span><br><span>@@ -147,8 +146,11 @@</span><br><span> while ((clframe = AST_LIST_REMOVE_HEAD(oldlist, list))) {</span><br><span> /* Only unlock if we own the lock */</span><br><span> if (clframe->channel == clframe->lock_frame->owner) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&clframe->lock_frame->mutex);</span><br><span> clframe->lock_frame->count = 0;</span><br><span> clframe->lock_frame->owner = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_cond_signal(&clframe->lock_frame->cond);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&clframe->lock_frame->mutex);</span><br><span> }</span><br><span> ast_free(clframe);</span><br><span> }</span><br><span>@@ -173,54 +175,11 @@</span><br><span> if (clframe->lock_frame->owner == oldchan) {</span><br><span> clframe->lock_frame->owner = newchan;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- /* We don't move requesters, because the thread stack is different */</span><br><span> clframe->channel = newchan;</span><br><span> }</span><br><span> AST_LIST_UNLOCK(list);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static void *lock_broker(void *unused)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- struct lock_frame *frame;</span><br><span style="color: hsl(0, 100%, 40%);">- struct timespec forever = { 1000000, 0 };</span><br><span style="color: hsl(0, 100%, 40%);">- for (;;) {</span><br><span style="color: hsl(0, 100%, 40%);">- int found_requester = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- /* Test for cancel outside of the lock */</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_testcancel();</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_TRAVERSE(&locklist, frame, entries) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (ao2_container_count(frame->requesters)) {</span><br><span style="color: hsl(0, 100%, 40%);">- found_requester++;</span><br><span style="color: hsl(0, 100%, 40%);">- ast_mutex_lock(&frame->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">- if (!frame->owner) {</span><br><span style="color: hsl(0, 100%, 40%);">- ast_cond_signal(&frame->cond);</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- ast_mutex_unlock(&frame->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_testcancel();</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- /* If there are no requesters, then wait for a signal */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!found_requester) {</span><br><span style="color: hsl(0, 100%, 40%);">- nanosleep(&forever, NULL);</span><br><span style="color: hsl(0, 100%, 40%);">- } else {</span><br><span style="color: hsl(0, 100%, 40%);">- sched_yield();</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- /* Not reached */</span><br><span style="color: hsl(0, 100%, 40%);">- return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-static int ast_channel_cmp_cb(void *obj, void *arg, int flags)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">- struct ast_channel *chan = obj, *cmp_args = arg;</span><br><span style="color: hsl(0, 100%, 40%);">- return strcasecmp(ast_channel_name(chan), ast_channel_name(cmp_args)) ? 0 : CMP_MATCH;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> static int get_lock(struct ast_channel *chan, char *lockname, int trylock)</span><br><span> {</span><br><span> struct ast_datastore *lock_store = ast_channel_datastore_find(chan, &lock_info, NULL);</span><br><span>@@ -290,17 +249,13 @@</span><br><span> AST_LIST_UNLOCK(&locklist);</span><br><span> return -1;</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- current->requesters = ao2_container_alloc_list(AO2_ALLOC_OPT_LOCK_MUTEX, 0,</span><br><span style="color: hsl(0, 100%, 40%);">- NULL, ast_channel_cmp_cb);</span><br><span style="color: hsl(0, 100%, 40%);">- if (!current->requesters) {</span><br><span style="color: hsl(0, 100%, 40%);">- ast_mutex_destroy(¤t->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_cond_destroy(¤t->cond);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_free(current);</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(120, 100%, 40%);">+ current->requesters = 0;</span><br><span> AST_LIST_INSERT_TAIL(&locklist, current, entries);</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Add to requester list */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+ current->requesters++;</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(¤t->mutex);</span><br><span> AST_LIST_UNLOCK(&locklist);</span><br><span> </span><br><span> /* Found lock or created one - now find or create the corresponding link in the channel */</span><br><span>@@ -337,44 +292,42 @@</span><br><span> * the same amount, before we'll release this one.</span><br><span> */</span><br><span> if (current->owner == chan) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* We're not a requester, we already have it */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+ current->requesters--;</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(¤t->mutex);</span><br><span> current->count++;</span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* Okay, we have both frames, so now we need to try to lock.</span><br><span style="color: hsl(0, 100%, 40%);">- *</span><br><span style="color: hsl(0, 100%, 40%);">- * Locking order: always lock locklist first. We need the</span><br><span style="color: hsl(0, 100%, 40%);">- * locklist lock because the broker thread counts whether</span><br><span style="color: hsl(0, 100%, 40%);">- * there are requesters with the locklist lock held, and we</span><br><span style="color: hsl(0, 100%, 40%);">- * need to hold it, so that when we send our signal, below,</span><br><span style="color: hsl(0, 100%, 40%);">- * to wake up the broker thread, it definitely will see that</span><br><span style="color: hsl(0, 100%, 40%);">- * a requester exists at that point in time. Otherwise, we</span><br><span style="color: hsl(0, 100%, 40%);">- * could add to the requesters after it has already seen that</span><br><span style="color: hsl(0, 100%, 40%);">- * that lock is unoccupied and wait forever for another signal.</span><br><span style="color: hsl(0, 100%, 40%);">- */</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_mutex_lock(¤t->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">- /* Add to requester list */</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_link(current->requesters, chan);</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_kill(broker_tid, SIGURG);</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /* Wait up to three seconds from now for LOCK. */</span><br><span> now = ast_tvnow();</span><br><span> timeout.tv_sec = now.tv_sec + 3;</span><br><span> timeout.tv_nsec = now.tv_usec * 1000;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!current->owner</span><br><span style="color: hsl(0, 100%, 40%);">- || (!trylock</span><br><span style="color: hsl(0, 100%, 40%);">- && !(res = ast_cond_timedwait(¤t->cond, ¤t->mutex, &timeout)))) {</span><br><span style="color: hsl(0, 100%, 40%);">- res = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ res = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ while (!trylock && !res && current->owner) {</span><br><span style="color: hsl(120, 100%, 40%);">+ res = ast_cond_timedwait(¤t->cond, ¤t->mutex, &timeout);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ if (current->owner) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* timeout;</span><br><span style="color: hsl(120, 100%, 40%);">+ * trylock; or</span><br><span style="color: hsl(120, 100%, 40%);">+ * cond_timedwait failed.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * either way, we fail to obtain the lock.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ res = -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ } else {</span><br><span> current->owner = chan;</span><br><span> current->count++;</span><br><span style="color: hsl(0, 100%, 40%);">- } else {</span><br><span style="color: hsl(0, 100%, 40%);">- res = -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ res = 0;</span><br><span> }</span><br><span> /* Remove from requester list */</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_unlink(current->requesters, chan);</span><br><span style="color: hsl(120, 100%, 40%);">+ current->requesters--;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (res && unloading)</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_cond_signal(¤t->cond);</span><br><span> ast_mutex_unlock(¤t->mutex);</span><br><span> </span><br><span> return res;</span><br><span>@@ -422,7 +375,10 @@</span><br><span> }</span><br><span> </span><br><span> if (--clframe->lock_frame->count == 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&clframe->lock_frame->mutex);</span><br><span> clframe->lock_frame->owner = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_cond_signal(&clframe->lock_frame->cond);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&clframe->lock_frame->mutex);</span><br><span> }</span><br><span> </span><br><span> ast_copy_string(buf, "1", len);</span><br><span>@@ -478,33 +434,33 @@</span><br><span> /* Module flag */</span><br><span> unloading = 1;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">- while ((current = AST_LIST_REMOVE_HEAD(&locklist, entries))) {</span><br><span style="color: hsl(0, 100%, 40%);">- /* If any locks are currently in use, then we cannot unload this module */</span><br><span style="color: hsl(0, 100%, 40%);">- if (current->owner || ao2_container_count(current->requesters)) {</span><br><span style="color: hsl(0, 100%, 40%);">- /* Put it back */</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_INSERT_HEAD(&locklist, current, entries);</span><br><span style="color: hsl(0, 100%, 40%);">- AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">- unloading = 0;</span><br><span style="color: hsl(0, 100%, 40%);">- return -1;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">- ast_mutex_destroy(¤t->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">- ao2_ref(current->requesters, -1);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_free(current);</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- /* No locks left, unregister functions */</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Make it impossible for new requesters to be added</span><br><span style="color: hsl(120, 100%, 40%);">+ * NOTE: channels could already be in get_lock() */</span><br><span> ast_custom_function_unregister(&lock_function);</span><br><span> ast_custom_function_unregister(&trylock_function);</span><br><span style="color: hsl(0, 100%, 40%);">- ast_custom_function_unregister(&unlock_function);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (broker_tid != AST_PTHREADT_NULL) {</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_cancel(broker_tid);</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_kill(broker_tid, SIGURG);</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_join(broker_tid, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(120, 100%, 40%);">+ AST_LIST_TRAVERSE(&locklist, current, entries) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+ while (current->owner || current->requesters) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* either the mutex is locked, or other parties are currently in get_lock,</span><br><span style="color: hsl(120, 100%, 40%);">+ * we need to wait for all of those to clear first */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_cond_wait(¤t->cond, ¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* At this point we know:</span><br><span style="color: hsl(120, 100%, 40%);">+ * 1. the lock has been released,</span><br><span style="color: hsl(120, 100%, 40%);">+ * 2. there are no requesters (nor should any be able to sneak in).</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_destroy(¤t->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_cond_destroy(¤t->cond);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_free(current);</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(120, 100%, 40%);">+ AST_LIST_HEAD_DESTROY(&locklist);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* At this point we can safely stop access to UNLOCK */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_custom_function_unregister(&unlock_function);</span><br><span> </span><br><span> return 0;</span><br><span> }</span><br><span>@@ -515,13 +471,6 @@</span><br><span> res |= ast_custom_function_register_escalating(&trylock_function, AST_CFE_READ);</span><br><span> res |= ast_custom_function_register_escalating(&unlock_function, AST_CFE_READ);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (ast_pthread_create_background(&broker_tid, NULL, lock_broker, NULL)) {</span><br><span style="color: hsl(0, 100%, 40%);">- ast_log(LOG_ERROR, "Failed to start lock broker thread. Unloading func_lock module.\n");</span><br><span style="color: hsl(0, 100%, 40%);">- broker_tid = AST_PTHREADT_NULL;</span><br><span style="color: hsl(0, 100%, 40%);">- unload_module();</span><br><span style="color: hsl(0, 100%, 40%);">- return AST_MODULE_LOAD_DECLINE;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> return res;</span><br><span> }</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15308">change 15308</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/15308"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ib3e560655befd3e99eec743687144f5569533379 </div>
<div style="display:none"> Gerrit-Change-Number: 15308 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Mark Petersen <bugs.digium.com@zombie.dk> </div>
<div style="display:none"> Gerrit-Reviewer: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Attention: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>