[Home]

Summary:ASTERISK-04869: [patch] [post 1.2] wrappers for memory allocation error handling
Reporter:Russell Bryant (russell)Labels:
Date Opened:2005-08-22 01:03:41Date Closed:2008-01-15 16:13:52.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast_malloc.rev5.patch
( 1) ast_malloc.rev6.patch
( 2) ast_malloc.rev7.diff
Description:This patch implements wrappers for memory allocation to standardize the error message that is printed out in the case of a memory allocation failure.
Comments:By: Tilghman Lesher (tilghman) 2005-08-22 02:00:17

1.  It's probably not a good idea to replace ast_cdr_alloc(), as we could potentially be doing other operations in the allocation of a CDR; we shouldn't remove encapsulation that we already have.

2.  Wouldn't it be easier and less error-prone simply to recommend that everybody start to use calloc() in CODING-GUIDELINES?  I'm all for making coding easier, but these extreme changes have had a tendency in the past to make CVS HEAD less stable and more buggy than it usually is.
By: Russell Bryant (russell) 2005-08-23 08:25:22

I put ast_cdr_alloc back in there.

We could recommend calloc in the guidelines, but I still kind of like it this way.  This produces one informative error message instead of having "Out of Memory" all over the codebase.
By: Russell Bryant (russell) 2005-09-16 13:29:44

Hm ... I was just thinking about this patch in regards to MALLOC_DEBUG.  With __ast_malloc defined with the AST_INLINE_API, then if the function was not being inlined, every single malloc would be reported as done in the same file.

Would it be better to define it as a macro?
By: Russell Bryant (russell) 2005-09-28 21:46:45

Uploaded rev3 to be current with CVS HEAD.

I have also implemented __ast_malloc as a macro instead of using the AST_INLINE_API for the reasons I mentioned in my last post.
By: Olle Johansson (oej) 2005-11-22 09:11:44.000-0600

Is this patch up to date with current cvs?

Would it make life easier to separate the patch into one that implements the new functionality and another that changes existing code to use the new functions? The latter one needs to stay closely up to date with coming changes to Asterisk cvs/svn...

/Housekeeping Mark II
By: Russell Bryant (russell) 2005-11-23 11:33:34.000-0600

updated to cvs head ...
By: Luigi Rizzo (rizzo) 2005-11-25 10:21:33.000-0600

i am certainly in favour of a wrapper for the malloc/strdup functions,
to produce a consistent behaviour (error logging) in case of failure.

HOWEVER, one thing which is really really to avoid, is that the macro has a hidden 'return' in it, because this would make the code very hard to follow.
So, i am against the *_ret() variants.

In terms of deployment, i second oej's comment, we should first commit
the implementation of the new functionality (ast_malloc() or whatever)
and then incrementally start using it.

In terms of implementation (rev.4), I am a bit unclear why you need
__ast_malloc() to be a macro - you only need a macro for ast_malloc()
so you can extract line and file numbers, all the rest can be done within
a function. Consider that the expensive part of malloc in a multithreaded
environment is the locking, so the extra function call matters little.

In any case, once we settle on an API, the implementation can be changed
easily.
By: Russell Bryant (russell) 2005-11-25 10:50:54.000-0600

I will work on the API to reflect your suggestions.  I'll leave the code conversion for after the API has been settled upon.  Thank you for taking a look at it!
By: Russell Bryant (russell) 2005-11-25 11:39:27.000-0600

An updated API has been provided for further comments.

I removed the *ret* variants and added wrappers for most of the functions for allocating memory.
By: Luigi Rizzo (rizzo) 2005-11-25 12:23:46.000-0600

rev.5 looks almost good. Three comments:
- the input to str[n]dup() is should be const char *, not char *

- why the names __ast_foo() have been changed to __astmm_foo() ?

- a common pattern in asterisk is    y= x ? strdup(x) : NULL;
 (written in the most various forms).
 It would be useful for ast_strdup to be able to handle a NULL argument
 and return a NULL without error condition in this case.
 The standard strdup is not able to handle a NULL argument, but given
 that this is our own API we could implement this.
By: Russell Bryant (russell) 2005-11-26 11:55:53.000-0600

rev6 uploaded.

- ast_str[n]dup now take a const char * as an argument.

- ast_str[n]dup can now handle a NULL argument.  They will return NULL without error.

- I changed the __ast_foo functions in astmm.c to __astmm_foo to make sure there was no confusion between those functions and the ones that are a part of this new API.  In rev6, I just changed the new functions to use a single underscore instead so astmm doesn't have to be changed.  I'm open to suggestions on the best way to avoid any confusion here.
By: Russell Bryant (russell) 2005-11-30 17:06:22.000-0600

rev7 fixes one small, but very critical typo.  :)
By: Russell Bryant (russell) 2005-12-12 22:57:56.000-0600

I am now maintaining this patch in a branch in svn.  It is in svn/asterisk/team/russell/ast_malloc
By: Russell Bryant (russell) 2005-12-25 19:53:23.000-0600

I have updated the coding guidelines in the ast_malloc branch accordingly.

The branch also contains some changes to the core C files from when I was testing things out.  For anyone looking at the changes, the important stuff is *only* inlcude/asterisk/utils.h and doc/CODING-GUIDELINES.
By: Russell Bryant (russell) 2006-01-10 18:02:21.000-0600

I have merged the changes discussed here into the trunk.
By: Digium Subversion (svnbot) 2008-01-15 16:06:20.000-0600

Repository: asterisk
Revision: 7452

A   team/russell/ast_malloc/

------------------------------------------------------------------------
r7452 | russell | 2008-01-15 16:06:20 -0600 (Tue, 15 Jan 2008) | 2 lines

create branch for memory allocation wrappers, discussed in issue ASTERISK-4869

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=7452
By: Digium Subversion (svnbot) 2008-01-15 16:06:23.000-0600

Repository: asterisk
Revision: 7455

U   team/russell/ast_malloc/include/asterisk/utils.h

------------------------------------------------------------------------
r7455 | russell | 2008-01-15 16:06:22 -0600 (Tue, 15 Jan 2008) | 2 lines

merge my patch from ASTERISK-4869 ...

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=7455
By: Digium Subversion (svnbot) 2008-01-15 16:13:52.000-0600

Repository: asterisk
Revision: 7952

U   trunk/doc/CODING-GUIDELINES
U   trunk/include/asterisk/utils.h

------------------------------------------------------------------------
r7952 | russell | 2008-01-15 16:13:51 -0600 (Tue, 15 Jan 2008) | 8 lines

Add wrappers for commonly used memory allocation functions.  These wrappers
add an automatically generated Asterisk log message if the allocation fails
for some reason.  Otherwise, they are functionally the same, with the
exception of ast_strdup and ast_strndup.  These functions have the added
ability to accept a NULL argument without error, which will just be ignored
without generating an error. The coding guidelines have also been updated to
reflect all of this information.  (issue ASTERISK-4869)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=7952