[asterisk-dev] tilghman: trunk r187483 - in /trunk: ./ include/asterisk/ main/
Russell Bryant
russell at digium.com
Thu Apr 9 20:45:51 CDT 2009
Some code level comments this time ...
On Apr 9, 2009, at 1:40 PM, SVN commits to the Digium repositories
wrote:
> Author: tilghman
> Date: Thu Apr 9 13:40:01 2009
> New Revision: 187483
>
> URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=187483
> Log:
> Merged revisions 187428 via svnmerge from
> https://origsvn.digium.com/svn/asterisk/branches/1.4
>
> ........
> r187428 | tilghman | 2009-04-09 13:08:20 -0500 (Thu, 09 Apr 2009) |
> 8 lines
>
> Race condition between ast_cli_command() and 'module unload' could
> cause a deadlock.
> Add lock timeouts to avoid this potential deadlock.
> (closes issue #14705)
> Reported by: jamessan
> Patches:
> 20090320__bug14705.diff.txt uploaded by tilghman (license 14)
> Tested by: jamessan
> ........
> --- trunk/include/asterisk/linkedlists.h (original)
> +++ trunk/include/asterisk/linkedlists.h Thu Apr 9 13:40:01 2009
> @@ -52,6 +52,18 @@
> ast_rwlock_wrlock(&(head)->lock)
>
> /*!
> + \brief Write locks a list, with timeout.
> + \param head This is a pointer to the list head structure
> + \param tv Pointer to a timeval structure
> +
> + This macro attempts to place an exclusive write lock in the
> + list head structure pointed to by head.
> + \retval 0 on success
> + \retval non-zero on failure
> +*/
> +#define AST_RWLIST_TIMEDWRLOCK(head,tv)
> ast_rwlock_timedwrlock(&(head)->lock, tv)
The comment block formatting almost everywhere else in Asterisk is of
the form:
/*!
*
*
*/
as opposed to:
/*!
*/
Please tweak it to match that format.
Also, for new API calls, please add the "\since 1.6.3" tag. This was
something Jeff started adding in his last round of API change
documentation, so we should all try to make a habit of using it when
we add a new API call (or change one).
You're missing a space after a comma.
Finally, your documentation and variable name indicate that the last
argument is a timeval. It's actually a timespec.
> +
> +/*!
> + \brief Read locks a list, with timeout.
> + \param head This is a pointer to the list head structure
> + \param tv Pointer to a timeval structure
> +
> + This macro attempts to place a read lock in the
> + list head structure pointed to by head.
> + \retval 0 on success
> + \retval non-zero on failure
> +*/
> +#define
> AST_RWLIST_TIMEDRDLOCK(head,tv) \
> + ast_rwlock_timedrdlock(&(head)->lock, tv)
Same comments as above.
> --- trunk/include/asterisk/lock.h (original)
> +++ trunk/include/asterisk/lock.h Thu Apr 9 13:40:01 2009
> @@ -1353,15 +1353,18 @@
> return res;
> }
>
> -static inline int _ast_rwlock_tryrdlock(ast_rwlock_t *t, const char
> *name,
> - const char *filename, int line, const char *func)
> +#define ast_rwlock_timedrdlock(a,b) \
> + _ast_rwlock_timedrdlock(a, # a, b, __FILE__, __LINE__,
> __PRETTY_FUNCTION__)
> +#define ast_rwlock_timedwrlock(a,b) \
> + _ast_rwlock_timedwrlock(a, # a, b, __FILE__, __LINE__,
> __PRETTY_FUNCTION__)
Missing a comma in "a,b".
> @@ -1449,6 +1472,139 @@
> ast_store_lock_info(AST_WRLOCK, filename, line, func, name, t);
> #endif
> }
> + res = pthread_rwlock_timedwrlock(&t->lock, abs_timeout);
> + if (!res) {
> + ast_reentrancy_lock(lt);
> + if (lt->reentrancy < AST_MAX_REENTRANCY) {
> + lt->file[lt->reentrancy] = filename;
> + lt->lineno[lt->reentrancy] = line;
> + lt->func[lt->reentrancy] = func;
> + lt->thread[lt->reentrancy] = pthread_self();
> + lt->reentrancy++;
> + }
> + ast_reentrancy_unlock(lt);
> + if (t->tracking) {
> + ast_mark_lock_acquired(t);
> + }
> + } else {
> +#ifdef HAVE_BKTR
> + if (lt->reentrancy) {
> + ast_reentrancy_lock(lt);
> + bt = <->backtrace[lt->reentrancy-1];
> + } else {
> + bt = NULL;
> + }
There is a ast_reentrancy_lock() here with no matching unlock().
>
> + if (t->tracking) {
> + ast_remove_lock_info(t, bt);
> + }
> +#else
> + if (t->tracking) {
> + ast_remove_lock_info(t);
> + }
> +#endif
> + __ast_mutex_logger("%s line %d (%s): Error obtaining read lock: %s
> \n",
> + filename, line, func, strerror(res));
> + DO_THREAD_CRASH;
> + }
> + return res;
> +}
>
> +
> +static inline int _ast_rwlock_tryrdlock(ast_rwlock_t *t, const char
> *name,
> + const char *filename, int line, const char *func)
> +{
> + int res;
> + struct ast_lock_track *lt = &t->track;
> +#ifdef HAVE_BKTR
> + struct ast_bt *bt = NULL;
> +#endif
> +
> +
> +#ifdef AST_MUTEX_INIT_W_CONSTRUCTORS
> + if ((t->lock) == ((pthread_rwlock_t) __AST_RWLOCK_INIT_VALUE)) {
> + /* Don't warn abount uninitialized lock.
> + * Simple try to initialize it.
> + * May be not needed in linux system.
> + */
> + res = __ast_rwlock_init(t->tracking, filename, line, func, name,
> t);
> + if ((t->lock) == ((pthread_rwlock_t) __AST_RWLOCK_INIT_VALUE)) {
> + __ast_mutex_logger("%s line %d (%s): Error: rwlock '%s' is
> uninitialized and unable to initialize.\n",
> + filename, line, func, name);
> + return res;
> + }
> + }
> +#endif /* AST_MUTEX_INIT_W_CONSTRUCTORS */
> +
> + if (t->tracking) {
> +#ifdef HAVE_BKTR
> + ast_reentrancy_lock(lt);
> + if (lt->reentrancy != AST_MAX_REENTRANCY) {
> + ast_bt_get_addresses(<->backtrace[lt->reentrancy]);
> + bt = <->backtrace[lt->reentrancy];
> + }
> + ast_reentrancy_unlock(lt);
I realize that this code was probably just copied from another similar
function, but I wonder if holding on to a struct ast_bt * outside of
the reentrancy lock could cause a problem. It means that we could be
trying to log a backtrace while another thread is overwriting the
contents of the ast_bt. At a minimum, it could lead to bogus
backtrace output. This same construct is used in a few places. Hmm ..
>
> + ast_store_lock_info(AST_RDLOCK, filename, line, func, name, t, bt);
> +#else
> + ast_store_lock_info(AST_RDLOCK, filename, line, func, name, t);
> +#endif
> + }
> +
> + if (!(res = pthread_rwlock_tryrdlock(&t->lock))) {
> + ast_reentrancy_lock(lt);
> + if (lt->reentrancy < AST_MAX_REENTRANCY) {
> + lt->file[lt->reentrancy] = filename;
> + lt->lineno[lt->reentrancy] = line;
> + lt->func[lt->reentrancy] = func;
> + lt->thread[lt->reentrancy] = pthread_self();
> + lt->reentrancy++;
> + }
> + ast_reentrancy_unlock(lt);
> + if (t->tracking) {
> + ast_mark_lock_acquired(t);
> + }
> + } else if (t->tracking) {
> + ast_mark_lock_failed(t);
> + }
> + return res;
> +}
> +
> @@ -3377,7 +3381,7 @@
> break;
> }
> }
> - AST_RWLIST_TRAVERSE_SAFE_END;
> + AST_RWLIST_TRAVERSE_SAFE_END
> AST_RWLIST_UNLOCK(&actions);
>
> return 0;
I know that technically this semicolon isn't needed, but I think
someone asked at one point to try to always use it, anyway, since it's
harmless, and made their syntax highlighting happy. I think emacs was
the editor in question.
>
--
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org
More information about the asterisk-dev
mailing list