Index: doc/tex/asterisk.tex =================================================================== --- doc/tex/asterisk.tex (revision 137185) +++ doc/tex/asterisk.tex (working copy) @@ -136,6 +136,8 @@ \input{phoneprov.tex} \chapter{Development} + \section{Coding guidelines} + \input{coding-guidelines.tex} \section{Backtrace} \input{backtrace.tex} @@ -153,7 +155,6 @@ %See http://www.asterisk.org/developers for more information % %callfiles.txt Asterisk callfiles using instruction -%CODING-GUIDELINES Guidelines for developers %externalivr.txt Documentation of the protocol used in externalivr() %modules.txt How Asterisk modules work %datastores.txt About channel data stores Index: doc/tex/coding-guidelines.tex =================================================================== --- doc/tex/coding-guidelines.tex (revision 0) +++ doc/tex/coding-guidelines.tex (revision 0) @@ -0,0 +1,1117 @@ +\subsection{Introduction} + +This section gives some basic indication on how the asterisk code +is structured. The first part covers the structure and style of +individual files. The second part (TO BE COMPLETED) covers the +overall code structure and the build architecture. + +Please read it to the end to understand in detail how the asterisk +code is organized, and to know how to extend asterisk or contribute +new code. + +We are looking forward to your contributions to Asterisk - the +Open Source PBX! As Asterisk is a large and in some parts very +time-sensitive application, the code base needs to conform to +a common set of coding rules so that many developers can enhance +and maintain the code. Code also needs to be reviewed and tested +so that it works and follows the general architecture and guide- +lines, and is well documented. + +Asterisk is published under a dual-licensing scheme by Digium. +To be accepted into the codebase, all non-trivial changes must be +licensed to Digium. For more information, see the electronic license +agreement on \url{http://bugs.digium.com/}. + +\subsection{Contributing patches} + +Patches should be in the form of a unified (-u) diff, made from a checkout +from subversion. + +\begin{astlisting} +\begin{verbatim} + /usr/src/asterisk$ svn diff > mypatch +\end{verbatim} +\end{astlisting} + +If you would like to only include changes to certain files in the patch, you +can list them in the "svn diff" command: + +\begin{astlisting} +\begin{verbatim} +/usr/src/asterisk$ svn diff somefile.c someotherfile.c > mypatch +\end{verbatim} +\end{astlisting} + +\subsection{Coding rules} + +\subsubsection{General rules} + +\begin{itemize} +\item All code, filenames, function names and comments must be in ENGLISH. + +\item Don't annotate your changes with comments like \verb|/* JMG 4/20/04 */|; + Comments should explain what the code does, not when something was changed + or who changed it. If you have done a larger contribution, make sure + that you are added to the \textbf{CREDITS} file. + +\item Don't make unnecessary whitespace changes throughout the code. + If you make changes, submit them to the tracker as separate patches + that only include whitespace and formatting changes. + +\item Don't use C++ type (//) comments. + +\item Try to match the existing formatting of the file you are working on. + +\item Use spaces instead of tabs when aligning in-line comments or \verb|#defines| (this makes + your comments aligned even if the code is viewed with another tabsize) +\end{itemize} + +\subsubsection{File structure and header inclusion} + +Every C source file should start with a proper copyright +and a brief description of the content of the file. +Following that, you should immediately put the following lines: + +\begin{astlisting} +\begin{verbatim} +#include "asterisk.h" +ASTERISK_FILE_VERSION(__FILE__, "$Revision: 127363 $") +\end{verbatim} +\end{astlisting} + +\begin{description} +\item[asterisk.h] resolves OS and compiler dependencies for the basic +set of unix functions (data types, system calls, basic I/O libraries) and the basic Asterisk APIs. + +\item[ASTERISK\_FILE\_VERSION()] stores in the executable information about the file. +\end{description} + +Next, you should \verb|#include| extra headers according to the functionality +that your file uses or implements. For each group of functions that +you use there is a common header, which covers OS header dependencies +and defines the 'external' API of those functions (the equivalent +of 'public' members of a class). As an example: + +\begin{itemize} + +\item \textbf{asterisk/module.h} + if you are implementing a module, this should be included in one + of the files that are linked with the module. + +\item \textbf{asterisk/io.h} + access to extra file I/O functions (stat, fstat, playing with + directories etc) + +\item \textbf{asterisk/network.h} + basic network I/O - all of the socket library, select/poll, + and asterisk-specific (usually either thread-safe or reentrant + or both) functions to play with socket addresses. + +\item \textbf{asterisk/app.h} + parsing of application arguments + +\item \textbf{asterisk/channel.h} + \verb|struct ast_channel| and functions to manipulate it +\end{itemize} + +For more information look at the headers in include/asterisk/ . +These files are usually self-sufficient, i.e. they recursively \verb|#include| +all the extra headers they need. + +The equivalent of 'private' members of a class are either directly in +the C source file, or in files named \verb|asterisk/mod_*.h| to make it clear +that they are not for inclusion by generic code. + +Keep the number of header files small by not including them unnecessarily. +Don't cut\&paste list of header files from other sources, but only include +those you really need. Apart from obvious cases (e.g. module.h which +is almost always necessary) write a short comment next to each \verb|#include| to +explain why you need it. + +\subsubsection{Declaration of functions and variables} + +\begin{itemize} +\item Do not declare variables mid-block (e.g. like recent GNU compilers support) + since it is harder to read and not portable to GCC 2.95 and others. + +\item Functions and variables that are not intended to be used outside the module + must be declared static. + +\item When reading integer numeric input with \verb|scanf()| (or variants), do \textbf{NOT} use '%i' + unless you specifically want to allow non-base-10 input; '%d' is always a better + choice, since it will not silently turn numbers with leading zeros into base-8. + +\item Strings that are coming from input should not be used as a first argument to + a formatted *\verb|printf| function. +\end{itemize} + +\subsubsection{Use the internal API} + +\begin{itemize} +\item Make sure you are aware of the string and data handling functions that exist + within Asterisk to enhance portability and in some cases to produce more + secure and thread-safe code. Check utils.c/utils.h for these. + +\item If you need to create a detached thread, use the \verb|ast_pthread_create_detached()| + normally or \verb|ast_pthread_create_detached_background()| for a thread with a smaller + stack size. This reduces the replication of the code to handle the \verb|pthread_attr_t| + structure. +\end{itemize} + +\subsubsection{Code formatting} + +Roughly, Asterisk code formatting guidelines are generally equivalent to the +following: + +\begin{astlisting} +\begin{verbatim} +# indent -i4 -ts4 -br -brs -cdw -lp -ce -nbfda -npcs -nprs -npsl -nbbo -saf -sai -saw -cs -l90 foo.c + +this means in verbose: + -i4: indent level 4 + -ts4: tab size 4 + -br: braces on if line + -brs: braces on struct decl line + -cdw: cuddle do while + -lp: line up continuation below parenthesis + -ce: cuddle else + -nbfda: dont break function decl args + -npcs: no space after function call names + -nprs: no space after parentheses + -npsl: dont break procedure type + -saf: space after for + -sai: space after if + -saw: space after while + -cs: space after cast + -ln90: line length 90 columns +\end{verbatim} +\end{astlisting} + +Function calls and arguments should be spaced in a consistent way across +the codebase. + +\begin{astlisting} +\begin{description} + \item[GOOD:] foo(arg1, arg2); + \item[BAD:] foo(arg1,arg2); + \item[BAD:] foo (arg1, arg2); + \item[BAD:] foo( arg1, arg2 ); + \item[BAD:] foo(arg1, arg2,arg3); +\end{description} +\end{astlisting} + +Don't treat keywords (\verb|if|, \verb|while|, \verb|do|, \verb|return|) as if they were functions; +leave space between the keyword and the expression used (if any). For \verb|return|, +don't even put parentheses around the expression, since they are not +required. + +There is no shortage of whitespace characters :-) Use them when they make +the code easier to read. For example: + +\begin{astlisting} +\begin{verbatim} + for (str=foo;str;str=str->next) +\end{verbatim} +\end{astlisting} + + +is harder to read than + +\begin{astlisting} +\begin{verbatim} + for (str = foo; str; str = str->next) +\end{verbatim} +\end{astlisting} + +Following are examples of how code should be formatted. + +\begin{itemize} + +\item Functions + +\begin{astlisting} +\begin{verbatim} + int foo(int a, char *s) + { + return 0; + } +\end{verbatim} +\end{astlisting} + +\item If statements + +\begin{astlisting} +\begin{verbatim} +if (foo) { + bar(); +} else { + blah(); +} +\end{verbatim} +\end{astlisting} + +\item Case statements + +\begin{astlisting} +\begin{verbatim} +switch (foo) { +case BAR: + blah(); + break; +case OTHER: + other(); + break; +} +\end{verbatim} +\end{astlisting} + +\item \textbf{No} nested statements without braces + + \begin{astlisting} + \begin{verbatim} + for (x = 0; x < 5; x++) + if (foo) + if (bar) + baz(); + + \end{verbatim} + \end{astlisting} +instead do: + \begin{astlisting} + \begin{verbatim} + for (x = 0; x < 5; x++) { + if (foo) { + if (bar) { + baz(); + } + } + } + \end{verbatim} + \end{astlisting} + +\item Always use braces around the statements following an \verb|if|/\verb|for|/\verb|while| construct, +even if not strictly necessary, as it reduces future possible problems. +Don't build code like this: + + \begin{astlisting} + \begin{verbatim} + if (foo) { + /* .... 50 lines of code ... */ + } else { + result = 0; + return; + } + \end{verbatim} + \end{astlisting} + +Instead, try to minimize the number of lines of code that need to be +indented, by only indenting the shortest case of the \verb|if| +statement, like so: + + \begin{astlisting} + \begin{verbatim} + if (!foo) { + result = 0; + return; + } + + .... 50 lines of code .... + \end{verbatim} + \end{astlisting} + +When this technique is used properly, it makes functions much easier to read +and follow, especially those with more than one or two 'setup' operations +that must succeed for the rest of the function to be able to execute. + +\item Labels/goto are acceptable + +Proper use of this technique may occasionally result in the need for a +label/goto combination so that error/failure conditions can exit the +function while still performing proper cleanup. This is not a bad thing! +Use of goto in this situation is encouraged, since it removes the need +for excess code indenting without requiring duplication of cleanup code. + +\item Never use an uninitialized variable + +Make sure you never use an uninitialized variable. The compiler will +usually warn you if you do so. However, do not go too far the other way, +and needlessly initialize variables that do not require it. If the first +time you use a variable in a function is to store a value there, then +initializing it at declaration is pointless, and will generate extra +object code and data in the resulting binary with no purpose. When in doubt, +trust the compiler to tell you when you need to initialize a variable; +if it does not warn you, initialization is not needed. + +\item Do not cast \verb|void *| + +Do not explicitly cast \verb|void *| into any other type, nor should you cast any +other type into \verb|void *|. Implicit casts to/from \verb|void *| are explicitly +allowed by the C specification. This means the results of \verb|malloc()|, \verb|calloc()|, +\verb|alloca()|, and similar functions do not \textbf{ever} need to be cast to a specific +type, and when you are passing a pointer to (for example) a callback function +that accepts a \verb|void *| you do not need to cast into that type. + +\end{itemize} + +\subsubsection{Function naming} + +All public functions (those not marked \verb|static|), must be named \verb|ast_()| +and have a descriptive name. + +As an example, suppose you wanted to take a local function \verb|find_feature|, defined +as static in a file, and used only in that file, and make it public, and use it +in other files. You will have to remove the "static" declaration and define a +prototype in an appropriate header file (usually in include/asterisk). A more +specific name should be given, such as \verb|ast_find_call_feature()|. + +\subsubsection{Variable function argument parsing} + +Functions with a variable amount of arguments need a 'sentinel' when called. +Newer GNU C compilers are fine if you use NULL for this. Older versions (pre 4) +don't like this. +You should use the constant SENTINEL. +This one is defined in \verb|include/asterisk/compiler.h| + +\subsubsection{Variable naming} + +\begin{description} +\item[Global variables] + +Name global variables (or local variables when you have a lot of them or +are in a long function) something that will make sense to aliens who +find your code in 100 years. All variable names should be in lower +case, except when following external APIs or specifications that normally +use upper- or mixed-case variable names; in that situation, it is +preferable to follow the external API/specification for ease of +understanding. + +Make some indication in the name of global variables which represent +options that they are in fact intended to be global. + e.g.: \linebreak +\verb|static char global_something[80]| + +\item[Don't use un-necessary typedef's] + +Don't use \verb|typedef| just to shorten the amount of typing; there is no substantial +benefit in this: + +\begin{astlisting} +\begin{verbatim} +struct foo { int bar; }; typedef struct foo foo_t; +\end{verbatim} +\end{astlisting} + +In fact, don't use 'variable type' suffixes at all; it's much preferable to +just type \verb|struct foo| rather than \verb|foo_s|. + +\item [Use enums instead of \#define where possible] + +Use enums rather than long lists of \verb|#define|-d numeric constants when possible; +this allows structure members, local variables and function arguments to +be declared as using the enum's type. For example: + +\begin{astlisting} +\begin{verbatim} +enum option { + OPT_FOO = 1, + OPT_BAR = 2, + OPT_BAZ = 4, +}; + +static enum option global_option; + +static handle_option(const enum option opt) +{ + ... +} +\end{verbatim} +\end{astlisting} + +Note: The compiler will \textbf{not} force you to pass an entry from the enum +as an argument to this function; this recommendation serves only to make +the code clearer and somewhat self-documenting. In addition, when using +switch/case blocks that switch on enum values, the compiler will warn +you if you forget to handle one or more of the enum values, which can be +handy. +\end{description} + +\subsubsection{String handling} + +Don't use strncpy for copying whole strings; it does not guarantee that the +output buffer will be null-terminated. Use \verb|ast_copy_string()| instead, which +is also slightly more efficient (and allows passing the actual buffer +size, which makes the code clearer). + +Don't use \verb|ast_copy_string()| (or any length-limited copy function) for copying +fixed (known at compile time) strings into buffers, if the buffer is something +that has been allocated in the function doing the copying. In that case, you +know at the time you are writing the code whether the buffer is large enough +for the fixed string or not, and if it's not, your code won't work anyway! +Use \verb|strcpy()| for this operation, or directly set the first two characters +of the buffer if you are just trying to store a one-character string in the +buffer. If you are trying to 'empty' the buffer, just store a single +\verb|NULL| character (\verb|\0|) in the first byte of the buffer; nothing else is +needed, and any other method is wasteful. + +In addition, if the previous operations in the function have already +determined that the buffer in use is adequately sized to hold the string +you wish to put into it (even if you did not allocate the buffer yourself), +use a direct \verb|strcpy()|, as it can be inlined and optimized to simple +processor operations, unlike \verb|ast_copy_string()|. + +\subsubsection{Use of functions} + +\begin{itemize} + +\item When making applications, always \verb|ast_strdupa(data)| to a local pointer if +you intend to parse the incoming data string. + +\begin{verbatim} + if (data) + mydata = ast_strdupa(data); +\end{verbatim} + +\item Use the argument parsing macros to declare arguments and parse them. + +\begin{astlisting} +\begin{verbatim} + AST_DECLARE_APP_ARGS(args, + AST_APP_ARG(arg1); + AST_APP_ARG(arg2); + AST_APP_ARG(arg3); + ); + parse = ast_strdupa(data); + AST_STANDARD_APP_ARGS(args, parse); +\end{verbatim} +\end{astlisting} + +\item Create generic code! + +If you do the same or a similar operation more than one time, make it a +function or macro. + +Make sure you are not duplicating any functionality already found in an +API call somewhere. If you are duplicating functionality found in +another static function, consider the value of creating a new API call +which can be shared. + +\end{itemize} + +\subsubsection{Handling of pointers and allocations} + +\begin{itemize} + +\item Dereference or localize pointers +Always dereference or localize pointers to things that are not yours like +channel members in a channel that is not associated with the current +thread and for which you do not have a lock. + +\begin{astlisting} +\begin{verbatim} + channame = ast_strdupa(otherchan->name); +\end{verbatim} +\end{astlisting} + +\item Use const on pointer arguments if possible + +Use const on pointer arguments which your function will not be modifying, as this +allows the compiler to make certain optimizations. In general, use \verb|const| +on any argument that you have no direct intention of modifying, as it can +catch logic/typing errors in your code when you use the argument variable +in a way that you did not intend. + +\item Do not create your own linked list code - reuse! + +As a common example of this point, make an effort to use the lockable +linked-list macros found in \verb|include/asterisk/linkedlists.h|. They are +efficient, easy to use and provide every operation that should be +necessary for managing a singly-linked list (if something is missing, +let us know!). Just because you see other open-coded list implementations +in the source tree is no reason to continue making new copies of +that code... There are also a number of common string manipulation +and timeval manipulation functions in \verb|asterisk/strings.h| and +\textbf{asterisk/time.h}; use them when possible. + +\item Avoid needless allocations + +Avoid needless \verb|malloc()|, \verb|strdup()| calls. If you only need the value in +the scope of your function try \verb|ast_strdupa()| or declare structs on the +stack and pass a pointer to them. However, be careful to \textbf{never} call +\verb|alloca()|, \verb|ast_strdupa()| or similar functions in the argument list +of a function you are calling; this can cause very strange stack +arrangements and produce unexpected behavior. + +\item Allocations for structures + +When allocating/zeroing memory for a structure, use code like this: + +\begin{astlisting} +\begin{verbatim} +struct foo *tmp; + +... + +tmp = ast_calloc(1, sizeof(*tmp)); +\end{verbatim} +\end{astlisting} + +Avoid the combination of \verb|ast_malloc()| and \verb|memset()|. Instead, always use +\verb|ast_calloc()|. This will allocate and zero the memory in a single operation. +In the case that uninitialized memory is acceptable, there should be a comment +in the code that states why this is the case. + +Using \verb|sizeof(*tmp)| instead of \verb|sizeof(struct foo)| eliminates duplication of the +'struct foo' identifier, which makes the code easier to read and also ensures +that if it is copy-and-pasted it won't require as much editing. + +The \verb|ast_*| family of functions for memory allocation are functionally the same. +They just add an Asterisk log error message in the case that the allocation +fails for some reason. This eliminates the need to generate custom messages +throughout the code to log that this has occurred. + +\item String Duplications + +The functions strdup and strndup can \textbf{not} accept a NULL argument. This results +in having code like this: + +\begin{astlisting} +\begin{verbatim} + if (str) + newstr = strdup(str); + else + newstr = NULL; +\end{verbatim} +\end{astlisting} + +However, the \verb|ast_strdup()| and \verb|ast_strdupa()| functions will happily accept a NULL +argument without generating an error. The same code can be written as: + +\begin{astlisting} +\begin{verbatim} + newstr = ast_strdup(str); +\end{verbatim} +\end{astlisting} + +Furthermore, it is unnecessary to have code that \verb|malloc()|/\verb|calloc()|'s for the length +of a string (+1 for the terminating \verb|\0|) and then using strncpy to copy the +copy the string into the resulting buffer. This is the exact same thing as +using \verb|ast_strdup()|. + +\end{itemize} + +\subsubsection{CLI Commands} + +New CLI commands should be named using the module's name, followed by a verb +and then any parameters that the command needs. For example: + +\begin{astlisting} +\begin{verbatim} +*CLI> iax2 show peer +\end{verbatim} +\end{astlisting} +\textbf{not} +\begin{astlisting} +\begin{verbatim} +*CLI> show iax2 peer +\end{verbatim} +\end{astlisting} + +\subsubsection{New dialplan applications/functions} + +There are two methods of adding functionality to the Asterisk +dialplan: applications and functions. Applications (found generally in +the apps/ directory) should be collections of code that interact with +a channel and/or user in some significant way. Functions (which can be +provided by any type of module) are used when the provided +functionality is simple... getting/retrieving a value, for +example. Functions should also be used when the operation is in no way +related to a channel (a computation or string operation, for example). + +Applications are registered and invoked using the +\verb|ast_register_application()| function}; see the \textbf{apps/app\_skel.c} file for an +example. + +Functions are registered using \verb|struct ast_custom_function| +structures and the \verb|ast_custom_function_register()| function. + + +\subsubsection{Doxygen API Documentation Guidelines} + +When writing Asterisk API documentation the following format should be +followed. Do not use the javadoc style. + +\begin{astlisting} +\begin{verbatim} +/*! + * \brief Do interesting stuff. + * + * \param thing1 interesting parameter 1. + * \param thing2 interesting parameter 2. + * + * This function does some interesting stuff. + * + * \retval zero on success + * \retval -1 on error. + */ +int ast_interesting_stuff(int thing1, int thing2) +{ + return 0; +} +\end{verbatim} +\end{astlisting} + +Notice the use of the \verb|\param|, \verb|\brief|, and \verb|\return| constructs. These should be +used to describe the corresponding pieces of the function being documented. +Also notice the blank line after the last \verb|\param| directive. All doxygen +comments must be in one \verb|/*! */| block. If the function or struct does not need +an extended description it can be left out. + +Please make sure to review the doxygen manual and make liberal use of the \verb|\a|, +\verb|\code|, \verb|c|, \verb|\b|, \verb|\note|, \verb|\li| and \verb|\e| +modifiers as appropriate. + +When documenting a \verb|static| function or an internal structure in a module, +use the \verb|\internal| modifier to ensure that the resulting documentation +explicitly says 'for internal use only'. + +Structures should be documented as follows. + +\begin{astlisting} +\begin{verbatim} +/*! + * \brief A very interesting structure. + */ +struct interesting_struct +{ + /*! \brief A data member. */ + int member1; + + int member2; /*!< \brief Another data member. */ +} +\end{verbatim} +\end{astlisting} + +Note that \verb|/*! */| blocks document the construct immediately following them +unless they are written, \verb|/*!< */|, in which case they document the construct +preceding them. + +It is very much preferred that documentation is not done inline, as done in +the previous example for member2. The first reason for this is that it tends +to encourage extremely brief, and often pointless, documentation since people +try to keep the comment from making the line extremely long. However, if you +insist on using inline comments, please indent the documentation with spaces! +That way, all of the comments are properly aligned, regardless of what tab +size is being used for viewing the code. + +\subsubsection{Finishing up before you submit your code} + +\begin{enumerate} + +\item Look at the code once more + +When you achieve your desired functionality, make another few refactor +passes over the code to optimize it. + +\item Read the patch + +Before submitting a patch, \textbf{read} the actual patch file to be sure that +all the changes you expect to be there are, and that there are no +surprising changes you did not expect. During your development, that +part of Asterisk may have changed, so make sure you compare with the +latest CVS. + +\item Listen to advice + +If you are asked to make changes to your patch, there is a good chance +the changes will introduce bugs, check it even more at this stage. +Also remember that the bug marshal or co-developer that adds comments +is only human, they may be in error :-) + +\item Optimize, optimize, optimize + +If you are going to reuse a computed value, save it in a variable +instead of recomputing it over and over. This can prevent you from +making a mistake in subsequent computations, making it easier to correct +if the formula has an error and may or may not help optimization but +will at least help readability. + +Just an example (so don't over analyze it, that'd be a shame): + +\begin{astlisting} +\begin{verbatim} +const char *prefix = "pre"; +const char *postfix = "post"; +char *newname; +char *name = "data"; + +if (name && (newname = alloca(strlen(name) + strlen(prefix) + strlen(postfix) + 3))) + snprintf(newname, strlen(name) + strlen(prefix) + strlen(postfix) + 3, "%s/%s/%s", + prefix, name, postfix); +\end{verbatim} +\end{astlisting} + +...vs this alternative: + +\begin{astlisting} +\begin{verbatim} +const char *prefix = "pre"; +const char *postfix = "post"; +char *newname; +char *name = "data"; +int len = 0; + +if (name && (len = strlen(name) + strlen(prefix) + strlen(postfix) + 3) && + (newname = alloca(len))) + snprintf(newname, len, "%s/%s/%s", prefix, name, postfix); +\end{verbatim} +\end{astlisting} + +\end{enumerate} + +\subsubsection{Creating new manager events?} + +If you create new AMI events, please read manager.txt. Do not re-use +existing headers for new purposes, but please re-use existing headers +for the same type of data. + +Manager events that signal a status are required to have one +event name, with a status header that shows the status. +The old style, with one event named "ThisEventOn" and another named +"ThisEventOff", is no longer approved. + +Check manager.txt for more information on manager and existing +headers. Please update this file if you add new headers. + +\subsubsection{Locking in Asterisk} + +\begin{enumerate} +\item[A.] Locking Fundamentals + +Asterisk is a heavily multithreaded application. It makes extensive +use of locking to ensure safe access to shared resources between +different threads. + +When more that one lock is involved in a given code path, there is the +potential for deadlocks. A deadlock occurs when a thread is stuck +waiting for a resource that it will never acquire. Here is a classic +example of a deadlock: + +\begin{center} +\begin{tabular}{ c | c } +Thread 1 & Thread 2 \\ +\hline +Holds Lock A & Holds Lock B \\ +Waiting for Lock B & Waiting for Lock A \\ +\end{tabular} +\end{center} + +In this case, there is a deadlock between threads 1 and 2. +This deadlock would have been avoided if both threads had +agreed that one must acquire Lock A before Lock B. + +In general, the fundamental rule for dealing with multiple locks is + +\begin{quote} + an order \textbf{must} be established to acquire locks, and then all threads + must respect that order when acquiring locks. +\end{quote} + +\item[A.1.] Establishing a locking order + +Because any ordering for acquiring locks is ok, one could establish +the rule arbitrarily, e.g. ordering by address, or by some other criterion. +The main issue, though, is defining an order that +\begin{itemize} + \item is easy to check at runtime; + \item reflects the order in which the code executes. +\end{itemize} +As an example, if a data structure B is only accessible through a +data structure A, and both require locking, then the natural order +is locking first A and then B. +As another example, if we have some unrelated data structures to +be locked in pairs, then a possible order can be based on the address +of the data structures themselves. + +\item[B.] Minding the boundary between channel drivers and the Asterisk core + +The \#1 cause of deadlocks in Asterisk is by not properly following the +locking rules that exist at the boundary between Channel Drivers and +the Asterisk core. The Asterisk core allocates an \verb|ast_channel|, and +Channel Drivers allocate "technology specific private data" (PVT) that is +associated with an \verb|ast_channel|. Typically, both the \verb|ast_channel| and +PVT have their own lock. There are \textbf{many} +code paths that require both objects to be locked. + +The locking order in this situation is the following: +\begin{enumerate} + \item ast\_channel + \item PVT +\end{enumerate} + +Channel Drivers implement the \verb|ast_channel_tech interface| to provide a +channel implementation for Asterisk. Most of the \verb|channel_tech| +interface callbacks are called with the associated \verb|ast_channel| +locked. When accessing technology specific data, the PVT can be locked +directly because the locking order is respected. + +\item[C.] Preventing lock ordering reversals. + +There are some code paths which make it extremely difficult to +respect the locking order. +Consider for example the following situation: + +\begin{enumerate} + \item A message comes in over the "network" + \item The Channel Driver (CD) monitor thread receives the message + \item The CD associates the message with a PVT and locks the PVT + \item While processing the message, the CD must do something that requires + locking the ast\_channel associated to the PVT +\end{enumerate} + +This is the point that must be handled carefully. +The following psuedo-code + +\begin{astlisting} +\begin{verbatim} + unlock(pvt); + lock(ast_channel); + lock(pvt); +\end{verbatim} +\end{astlisting} + +is \textbf{not} correct for two reasons: + +\begin{enumerate} + +\item first and foremost, unlocking the PVT means that other threads + can acquire the lock and believe it is safe to modify the + associated data. When reacquiring the lock, the original thread + might find unexpected changes in the protected data structures. + This essentially means that the original thread must behave as if + the lock on the pvt was not held, in which case it could have + released it itself altogether; + +\item Asterisk uses the so called "recursive" locks, which allow a thread + to issue a \verb|lock()| call multiple times on the same lock. Recursive + locks count the number of calls, and they require an equivalent + number of \verb|unlock()| to be actually released. + + For this reason, just calling \verb|unlock()| once does not guarantee that the + lock is actually released -- it all depends on how many times \verb|lock()| + was called before. +\end{enumerate} + +An alternative, but still incorrect, construct is widely used in +the asterisk code to try and improve the situation: + +\begin{astlisting} +\begin{verbatim} + while (trylock(ast_channel) == FAILURE) { + unlock(pvt); + usleep(1); /* yield to other threads */ + lock(pvt); + } +\end{verbatim} +\end{astlisting} + +Here the \verb|trylock()| is non blocking, so we do not deadlock if the \verb|ast_channel| +is already locked by someone else: in this case, we try to unlock the PVT +(which happens only if the PVT lock counter is 1), yield the CPU to +give other threads a chance to run, and then acquire the lock again. + +This code is not correct for two reasons: +\begin{enumerate} + + \item Same as in the previous example, it releases the lock when the thread + probably did not expect it; + \item If the PVT lock counter is greater than 1 we will not + really release the lock on the PVT. We might be lucky and have the + other contender actually release the lock itself, and so we will "win" + the race, but if both contenders have their lock counts > 1 then + they will loop forever (basically replacing deadlock with livelock). +\end{enumerate} + +Another variant of this code is the following: + +\begin{astlisting} +\begin{verbatim} + if (trylock(ast_channel) == FAILURE) { + unlock(pvt); + lock(ast_channel); + lock(pvt); + } +\end{verbatim} +\end{astlisting} + +which has the same issues as the \verb|while(trylock...)| code, but just +deadlocks instead of looping forever in case of lock \verb|counts > 1|. + +The deadlock/livelock could be in principle spared if one had an +\verb|unlock_all()| function that calls unlock as many times as needed to +actually release the lock, and reports the count. Then we could do: + +\begin{astlisting} +\begin{verbatim} + if (trylock(ast_channel) == FAILURE) { + n = unlock_all(pvt); + lock(ast_channel) + while (n-- > 0) lock(pvt); + } +\end{verbatim} +\end{astlisting} + +The issue with unexpected unlocks remains, though. + +\item[D.] Locking multiple channels. + +The next situation to consider is what to do when you need a lock on +multiple ast\_channels (or multiple unrelated data structures). + +If we are sure that we do not hold any of these locks, then the +following construct is sufficient: + +\begin{astlisting} +\begin{verbatim} + lock(MIN(chan1, chan2)); + lock(MAX(chan1, chan2)); +\end{verbatim} +\end{astlisting} + +That type of code would follow an established locking order of always +locking the channel that has a lower address first. Also keep in mind +that to use this construct for channel locking, one would have to go +through the entire codebase to ensure that when two channels are locked, +this locking order is used. + + However, if we enter the above section of code with some lock held +(which would be incorrect using non-recursive locks, but is completely +legal using recursive mutexes) then the locking order is not guaranteed +anymore because it depends on which locks we already hold. So we have +to go through the same tricks used for the channel+PVT case. + +\item[E.] Recommendations + +As you can see from the above discussion, getting locking right is all +but easy. So please follow these recommendations when using locks: + +\begin{itemize} + +\item Use locks only when really necessary + + Please try to use locks only when strictly necessary, and only for + the minimum amount of time required to run critical sections of code. + A common use of locks in the current code is to protect a data structure + from being released while you use it. + With the use of reference-counted objects (astobj2) this should not be + necessary anymore. + +\item Do not sleep while holding a lock + + If possible, do not run any blocking code while holding a lock, + because you will also block other threads trying to access the same + lock. In many cases, you can hold a reference to the object to avoid + that it is deleted while you sleep, perhaps set a flag in the object + itself to report other threads that you have some pending work to + complete, then release and acquire the lock around the blocking path, + checking the status of the object after you acquire the lock to make + sure that you can still perform the operation you wanted to. + +\item Try not to exploit the 'recursive' feature of locks. + + Recursive locks are very convenient when coding, as you don't have to + worry, when entering a section of code, whether or not you already + hold the lock -- you can just protect the section with a lock/unlock + pair and let the lock counter track things for you. + + But as you have seen, exploiting the features of recursive locks + make it a lot harder to implement proper deadlock avoidance strategies. + So please try to analyse your code and determine statically whether you + already hold a lock when entering a section of code. + + If you need to call some function \verb|foo()| with and without a lock held, + you could define two function as below: + + \begin{astlisting} + \begin{verbatim} + foo_locked(...) { + ... do something, assume lock held + } + + foo(...) { + lock(xyz) + ret = foo_locked(...) + unlock(xyz) + return ret; + } + \end{verbatim} + \end{astlisting} + + and call them according to the needs. + +\item Document locking rules. + + Please document the locking order rules are documented for every + lock introduced into Asterisk. This is done almost nowhere in the + existing code. However, it will be expected to be there for newly + introduced code. Over time, this information should be added for + all of the existing lock usage. +\end{itemize} + +\end{enumerate} + + +\subsection{Build Architecture} + +The asterisk build architecture relies on autoconf to detect the +system configuration, and on a locally developed tool (menuselect) to +select build options and modules list, and on gmake to do the build. + +The first step, usually to be done soon after a checkout, is running +"./configure", which will store its findings in two files: + +\begin{description} + \item[include/asterisk/autoconfig.h] + + contains C macros, normally \verb|#define HAVE_FOO| or \verb|HAVE_FOO_H| , + for all functions and headers that have been detected at build time. + These are meant to be used by C or C++ source files. + + \item[makeopts] + + contains variables that can be used by Makefiles. + In addition to the usual CC, LD, ... variables pointing to + the various build tools, and prefix, includedir ... which are + useful for generic compiler flags, there are variables + for each package detected. + These are normally of the form \verb|FOO_INCLUDE=... FOO_LIB=...| + \verb|FOO_DIR=...| indicating, for each package, the useful libraries + and header files. +\end{description} + +The next step is to run "make menuselect", to extract the dependencies existing +between files and modules, and to store build options. +menuselect produces two files, both to be read by the Makefile: + +\begin{description} + \item[menuselect.makeopts] + + Contains for each subdirectory a list of modules that must be + excluded from the build, plus some additional informatiom. + + \item[menuselect.makedeps] + + Contains, for each module, a list of packages it depends on. + For each of these packages, we can collect the relevant INCLUDE + and LIB files from makeopts. This file is based on information + in the .c source code files for each module. +\end{description} + +The top level Makefile is in charge of setting up the build environment, +creating header files with build options, and recursively invoking the +subdir Makefiles to produce modules and the main executable. + +The sources are split in multiple directories, more or less divided by +module type (apps/ channels/ funcs/ res/ ...) or by function, for the main +binary (main/ pbx/). + +