mirror of https://github.com/asterisk/asterisk
You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
925 lines
34 KiB
925 lines
34 KiB
--------------------------------------
|
|
== Asterisk Coding Guidelines ==
|
|
--------------------------------------
|
|
|
|
This document 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 http://bugs.digium.com/.
|
|
|
|
Patches should be in the form of a unified (-u) diff, made from a checkout
|
|
from subversion.
|
|
|
|
/usr/src/asterisk$ svn diff > mypatch
|
|
|
|
If you would like to only include changes to certain files in the patch, you
|
|
can list them in the "svn diff" command:
|
|
|
|
/usr/src/asterisk$ svn diff somefile.c someotherfile.c > mypatch
|
|
|
|
-----------------------------------
|
|
== PART ONE: CODING GUIDELINES ==
|
|
-----------------------------------
|
|
|
|
* General rules
|
|
---------------
|
|
|
|
- All code, filenames, function names and comments must be in ENGLISH.
|
|
|
|
- Don't annotate your changes with comments like "/* 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 CREDITS file.
|
|
|
|
- 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.
|
|
|
|
- Don't use C++ type (//) comments.
|
|
|
|
- Try to match the existing formatting of the file you are working on.
|
|
|
|
- Use spaces instead of tabs when aligning in-line comments or #defines (this makes
|
|
your comments aligned even if the code is viewed with another tabsize)
|
|
|
|
* 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:
|
|
|
|
#include "asterisk.h"
|
|
ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
|
|
|
|
"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.
|
|
ASTERISK_FILE_VERSION() stores in the executable information
|
|
about the file.
|
|
|
|
Next, you should #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:
|
|
|
|
asterisk/module.h
|
|
if you are implementing a module, this should be included in one
|
|
of the files that are linked with the module.
|
|
|
|
asterisk/io.h
|
|
access to extra file I/O functions (stat, fstat, playing with
|
|
directories etc)
|
|
|
|
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.
|
|
|
|
asterisk/app.h
|
|
parsing of application arguments
|
|
|
|
asterisk/channel.h
|
|
struct ast_channel and functions to manipulate it
|
|
|
|
For more information look at the headers in include/asterisk/ .
|
|
These files are usually self-sufficient, i.e. they recursively #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 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 #include to
|
|
explain why you need it.
|
|
|
|
|
|
* Declaration of functions and variables
|
|
----------------------------------------
|
|
|
|
- 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.
|
|
|
|
- Functions and variables that are not intended to be used outside the module
|
|
must be declared static.
|
|
|
|
- When reading integer numeric input with scanf (or variants), do _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.
|
|
|
|
- Strings that are coming from input should not be used as a first argument to
|
|
a formatted *printf function.
|
|
|
|
* Use the internal API
|
|
----------------------
|
|
|
|
- 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.
|
|
|
|
- If you need to create a detached thread, use the ast_pthread_create_detached()
|
|
normally or ast_pthread_create_detached_background() for a thread with a smaller
|
|
stack size. This reduces the replication of the code to handle the pthread_attr_t
|
|
structure.
|
|
|
|
* Code formatting
|
|
-----------------
|
|
|
|
Roughly, Asterisk code formatting guidelines are generally equivalent to the
|
|
following:
|
|
|
|
# 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
|
|
|
|
Function calls and arguments should be spaced in a consistent way across
|
|
the codebase.
|
|
GOOD: foo(arg1, arg2);
|
|
BAD: foo(arg1,arg2);
|
|
BAD: foo (arg1, arg2);
|
|
BAD: foo( arg1, arg2 );
|
|
BAD: foo(arg1, arg2,arg3);
|
|
|
|
Don't treat keywords (if, while, do, return) as if they were functions;
|
|
leave space between the keyword and the expression used (if any). For '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:
|
|
|
|
for (str=foo;str;str=str->next)
|
|
|
|
is harder to read than
|
|
|
|
for (str = foo; str; str = str->next)
|
|
|
|
Following are examples of how code should be formatted.
|
|
|
|
- Functions:
|
|
int foo(int a, char *s)
|
|
{
|
|
return 0;
|
|
}
|
|
|
|
- If statements:
|
|
if (foo) {
|
|
bar();
|
|
} else {
|
|
blah();
|
|
}
|
|
|
|
- Case statements:
|
|
switch (foo) {
|
|
case BAR:
|
|
blah();
|
|
break;
|
|
case OTHER:
|
|
other();
|
|
break;
|
|
}
|
|
|
|
- No nested statements without braces, e.g.:
|
|
|
|
for (x = 0; x < 5; x++)
|
|
if (foo)
|
|
if (bar)
|
|
baz();
|
|
|
|
instead do:
|
|
for (x = 0; x < 5; x++) {
|
|
if (foo) {
|
|
if (bar) {
|
|
baz();
|
|
}
|
|
}
|
|
}
|
|
|
|
- Always use braces around the statements following an if/for/while construct,
|
|
even if not strictly necessary, as it reduces future possible problems.
|
|
|
|
- Don't build code like this:
|
|
|
|
if (foo) {
|
|
/* .... 50 lines of code ... */
|
|
} else {
|
|
result = 0;
|
|
return;
|
|
}
|
|
|
|
Instead, try to minimize the number of lines of code that need to be
|
|
indented, by only indenting the shortest case of the 'if'
|
|
statement, like so:
|
|
|
|
if (!foo) {
|
|
result = 0;
|
|
return;
|
|
}
|
|
|
|
.... 50 lines of code ....
|
|
|
|
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.
|
|
|
|
- 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.
|
|
|
|
- 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.
|
|
|
|
- Do not cast 'void *'
|
|
Do not explicitly cast 'void *' into any other type, nor should you cast any
|
|
other type into 'void *'. Implicit casts to/from 'void *' are explicitly
|
|
allowed by the C specification. This means the results of malloc(), calloc(),
|
|
alloca(), and similar functions do not _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 'void *' you do not need to cast into that type.
|
|
|
|
* Function naming
|
|
-----------------
|
|
|
|
All public functions (those not marked 'static'), must be named "ast_<something>"
|
|
and have a descriptive name.
|
|
|
|
As an example, suppose you wanted to take a local function "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 "ast_find_call_feature".
|
|
|
|
* 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 include/asterisk/compiler.h
|
|
|
|
* Variable naming
|
|
-----------------
|
|
|
|
- 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.: static char global_something[80]
|
|
|
|
- Don't use un-necessary typedef's
|
|
Don't use 'typedef' just to shorten the amount of typing; there is no substantial
|
|
benefit in this:
|
|
struct foo { int bar; }; typedef struct foo foo_t;
|
|
|
|
In fact, don't use 'variable type' suffixes at all; it's much preferable to
|
|
just type 'struct foo' rather than 'foo_s'.
|
|
|
|
- Use enums instead of #define where possible
|
|
Use enums rather than long lists of #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:
|
|
|
|
enum option {
|
|
OPT_FOO = 1,
|
|
OPT_BAR = 2,
|
|
OPT_BAZ = 4,
|
|
};
|
|
|
|
static enum option global_option;
|
|
|
|
static handle_option(const enum option opt)
|
|
{
|
|
...
|
|
}
|
|
|
|
Note: The compiler will _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.
|
|
|
|
* String handling
|
|
-----------------
|
|
|
|
Don't use strncpy for copying whole strings; it does not guarantee that the
|
|
output buffer will be null-terminated. Use 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 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 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
|
|
NULL character ('\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 strcpy(), as it can be inlined and optimized to simple
|
|
processor operations, unlike ast_copy_string().
|
|
|
|
* Use of functions
|
|
------------------
|
|
|
|
When making applications, always ast_strdupa(data) to a local pointer if
|
|
you intend to parse the incoming data string.
|
|
|
|
if (data)
|
|
mydata = ast_strdupa(data);
|
|
|
|
|
|
- Use the argument parsing macros to declare arguments and parse them, i.e.:
|
|
|
|
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);
|
|
|
|
- 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.
|
|
|
|
* Handling of pointers and allocations
|
|
--------------------------------------
|
|
|
|
- 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.
|
|
channame = ast_strdupa(otherchan->name);
|
|
|
|
- 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 '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.
|
|
|
|
- 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 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 asterisk/strings.h and asterisk/time.h;
|
|
use them when possible.
|
|
|
|
- Avoid needless allocations!
|
|
Avoid needless malloc(), strdup() calls. If you only need the value in
|
|
the scope of your function try ast_strdupa() or declare structs on the
|
|
stack and pass a pointer to them. However, be careful to _never_ call
|
|
alloca(), 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.
|
|
|
|
-Allocations for structures
|
|
When allocating/zeroing memory for a structure, use code like this:
|
|
|
|
struct foo *tmp;
|
|
|
|
...
|
|
|
|
tmp = ast_calloc(1, sizeof(*tmp));
|
|
|
|
Avoid the combination of ast_malloc() and memset(). Instead, always use
|
|
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 sizeof(*tmp) instead of 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 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.
|
|
|
|
-String Duplications
|
|
|
|
The functions strdup and strndup can *not* accept a NULL argument. This results
|
|
in having code like this:
|
|
|
|
if (str)
|
|
newstr = strdup(str);
|
|
else
|
|
newstr = NULL;
|
|
|
|
However, the ast_strdup and ast_strdupa functions will happily accept a NULL
|
|
argument without generating an error. The same code can be written as:
|
|
|
|
newstr = ast_strdup(str);
|
|
|
|
Furthermore, it is unnecessary to have code that malloc/calloc's for the length
|
|
of a string (+1 for the terminating '\0') and then using strncpy to copy the
|
|
copy the string into the resulting buffer. This is the exact same thing as
|
|
using ast_strdup.
|
|
|
|
* 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:
|
|
|
|
*CLI> iax2 show peer <peername>
|
|
|
|
not
|
|
|
|
*CLI> show iax2 peer <peername>
|
|
|
|
* 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
|
|
ast_register_application function; see the apps/app_skel.c file for an
|
|
example.
|
|
|
|
Functions are registered using 'struct ast_custom_function'
|
|
structures and the ast_custom_function_register function.
|
|
|
|
* Doxygen API Documentation Guidelines
|
|
--------------------------------------
|
|
|
|
When writing Asterisk API documentation the following format should be
|
|
followed. Do not use the javadoc style.
|
|
|
|
/*!
|
|
* \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;
|
|
}
|
|
|
|
Notice the use of the \param, \brief, and \return constructs. These should be
|
|
used to describe the corresponding pieces of the function being documented.
|
|
Also notice the blank line after the last \param directive. All doxygen
|
|
comments must be in one /*! */ 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 \a,
|
|
\code, \c, \b, \note, \li and \e modifiers as appropriate.
|
|
|
|
When documenting a 'static' function or an internal structure in a module,
|
|
use the \internal modifier to ensure that the resulting documentation
|
|
explicitly says 'for internal use only'.
|
|
|
|
Structures should be documented as follows.
|
|
|
|
/*!
|
|
* \brief A very interesting structure.
|
|
*/
|
|
struct interesting_struct
|
|
{
|
|
/*! \brief A data member. */
|
|
int member1;
|
|
|
|
int member2; /*!< \brief Another data member. */
|
|
}
|
|
|
|
Note that /*! */ blocks document the construct immediately following them
|
|
unless they are written, /*!< */, 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.
|
|
|
|
* Finishing up before you submit your code
|
|
------------------------------------------
|
|
|
|
- Look at the code once more
|
|
When you achieve your desired functionality, make another few refactor
|
|
passes over the code to optimize it.
|
|
|
|
- Read the patch
|
|
Before submitting a patch, *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.
|
|
|
|
- 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 :-)
|
|
|
|
- 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):
|
|
|
|
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);
|
|
|
|
...vs this alternative:
|
|
|
|
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);
|
|
|
|
* 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.
|
|
|
|
* Locking in Asterisk
|
|
-----------------------------
|
|
|
|
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:
|
|
|
|
Thread 1 Thread 2
|
|
------------ ------------
|
|
Holds Lock A Holds Lock B
|
|
Waiting for Lock B Waiting for Lock A
|
|
|
|
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
|
|
|
|
an order _must_ be established to acquire locks, and then all threads
|
|
must respect that order when acquiring locks.
|
|
|
|
|
|
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
|
|
i) is easy to check at runtime;
|
|
ii) reflects the order in which the code executes.
|
|
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.
|
|
|
|
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 ast_channel, and
|
|
Channel Drivers allocate "technology specific private data" (PVT) that is
|
|
associated with an ast_channel. Typically, both the ast_channel and
|
|
PVT have their own lock. There are _many_
|
|
code paths that require both objects to be locked.
|
|
|
|
The locking order in this situation is the following:
|
|
|
|
1) ast_channel
|
|
2) PVT
|
|
|
|
Channel Drivers implement the ast_channel_tech interface to provide a
|
|
channel implementation for Asterisk. Most of the channel_tech
|
|
interface callbacks are called with the associated ast_channel
|
|
locked. When accessing technology specific data, the PVT can be locked
|
|
directly because the locking order is respected.
|
|
|
|
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:
|
|
|
|
1) A message comes in over the "network"
|
|
2) The Channel Driver (CD) monitor thread receives the message
|
|
3) The CD associates the message with a PVT and locks the PVT
|
|
4) While processing the message, the CD must do something that requires
|
|
locking the ast_channel associated to the PVT
|
|
|
|
This is the point that must be handled carefully.
|
|
The following psuedo-code
|
|
|
|
unlock(pvt);
|
|
lock(ast_channel);
|
|
lock(pvt);
|
|
|
|
is _not_ correct for two reasons:
|
|
|
|
i) 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;
|
|
|
|
ii) Asterisk uses the so called "recursive" locks, which allow a thread
|
|
to issue a lock() call multiple times on the same lock. Recursive
|
|
locks count the number of calls, and they require an equivalent
|
|
number of unlock() to be actually released.
|
|
|
|
For this reason, just calling unlock() once does not guarantee that the
|
|
lock is actually released -- it all depends on how many times lock()
|
|
was called before.
|
|
|
|
An alternative, but still incorrect, construct is widely used in
|
|
the asterisk code to try and improve the situation:
|
|
|
|
while (trylock(ast_channel) == FAILURE) {
|
|
unlock(pvt);
|
|
usleep(1); /* yield to other threads */
|
|
lock(pvt);
|
|
}
|
|
|
|
Here the trylock() is non blocking, so we do not deadlock if the 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:
|
|
i) same as in the previous example, it releases the lock when the thread
|
|
probably did not expect it;
|
|
ii) 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).
|
|
|
|
Another variant of this code is the following:
|
|
|
|
if (trylock(ast_channel) == FAILURE) {
|
|
unlock(pvt);
|
|
lock(ast_channel);
|
|
lock(pvt);
|
|
}
|
|
|
|
which has the same issues as the while(trylock...) code, but just
|
|
deadlocks instead of looping forever in case of lock counts > 1.
|
|
|
|
The deadlock/livelock could be in principle spared if one had an
|
|
unlock_all() function that calls unlock as many times as needed to
|
|
actually release the lock, and reports the count. Then we could do:
|
|
|
|
if (trylock(ast_channel) == FAILURE) {
|
|
n = unlock_all(pvt);
|
|
lock(ast_channel)
|
|
while (n-- > 0) lock(pvt);
|
|
}
|
|
|
|
The issue with unexpected unlocks remains, though.
|
|
|
|
C) 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:
|
|
|
|
lock(MIN(chan1, chan2));
|
|
lock(MAX(chan1, chan2));
|
|
|
|
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.
|
|
|
|
D) Recommendations
|
|
|
|
As you can see from the above discussion, getting locking right is all
|
|
but easy. So please follow these recommendations when using locks:
|
|
|
|
*) 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.
|
|
|
|
*) 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.
|
|
|
|
*) 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 foo() with and without a lock held,
|
|
you could define two function as below:
|
|
foo_locked(...) {
|
|
... do something, assume lock held
|
|
}
|
|
|
|
foo(...) {
|
|
lock(xyz)
|
|
ret = foo_locked(...)
|
|
unlock(xyz)
|
|
return ret;
|
|
}
|
|
and call them according to the needs.
|
|
|
|
*) 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.
|
|
|
|
-----------------------------------------------------------------------
|
|
|
|
|
|
------------------------------------
|
|
== PART TWO: 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:
|
|
|
|
+ include/asterisk/autoconfig.h
|
|
contains C macros, normally #define HAVE_FOO or 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.
|
|
|
|
+ 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 FOO_INCLUDE=... FOO_LIB=...
|
|
FOO_DIR=... indicating, for each package, the useful libraries
|
|
and header files.
|
|
|
|
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:
|
|
|
|
+ menuselect.makeopts
|
|
Contains for each subdirectory a list of modules that must be
|
|
excluded from the build, plus some additional informatiom.
|
|
+ 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.
|
|
|
|
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/).
|
|
|
|
|
|
TO BE COMPLETED
|
|
|
|
|
|
-----------------------------------------------
|
|
Welcome to the Asterisk development community!
|
|
Meet you on the asterisk-dev mailing list.
|
|
Subscribe at http://lists.digium.com!
|
|
|
|
-- The Asterisk.org Development Team
|