[asterisk-dev] [Code Review] Out-of-tree modules can fail because of compiler flag differences.

[Kevin Fleming]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/508/#review1617
-----------------------------------------------------------


I can't review every line of code, and of course this needs more that just compile-time testing, but the design looks sound and is a definite improvement over the previous iteration.


/branches/1.4/include/asterisk/lock.h
<https://reviewboard.asterisk.org/r/508/#comment3619>

    This code is correct C, as the typedef will forward-declare 'struct ast_mutex_info'. However, someone reading the code will not know that the structure is going to be defined later.
    
    I would suggest a fairly lengthy comment above this typedef that explains where the structure will actually be defined, and why this technique is being used. It's completely logical and the correct way to do it, but could use some explanation to avoid future questions.



/branches/1.4/include/asterisk/lock.h
<https://reviewboard.asterisk.org/r/508/#comment3618>

    This is really nit-picky, but these function declarations should be *before* the macros that use them. The compiler doesn't care, but it's better for the reader.



/branches/1.4/include/asterisk/lock.h
<https://reviewboard.asterisk.org/r/508/#comment3622>

    Now that I think about this some more, there is a fatal flaw here. If the code in lock.c is compiled with DEBUG_THREADS enabled, then the code there will expect all these fields to exist. If a module is compiled with DEBUG_THREADS disabled, then an instance of 'ast_mutex_info' (which it passes via pointer to an API call) will not in fact have these fields, and thus access to them will cause very broken behavior.
    
    I believe what will have to be done here is to have only one definition of ast_mutex_info, and rely on the fact that a module compiled without DEBUG_THREADS defined will end up with 'track' set to zero in each of its instances of this structure, so the core code won't bother spending any time tracking those lock/unlock calls. If the core code is compiled without DEBUG_THREADS and the module is compiled with DEBUG_THREADS, the core code won't try to use the excess fields even though they will be present.



/branches/1.4/main/lock.c
<https://reviewboard.asterisk.org/r/508/#comment3620>

    This is general locking, not specific to channels.



/branches/1.4/main/lock.c
<https://reviewboard.asterisk.org/r/508/#comment3621>

    Nasty red squares... get rid of all of them :-)


- Kevin


On 2010-03-01 15:19:22, qwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/508/
> -----------------------------------------------------------
> 
> (Updated 2010-03-01 15:19:22)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When compiling modules out-of-tree, there is a large potential for ABI breakage, due to changing compiler flags.  If Asterisk is compiled with DEBUG_THREADS, and an out-of-tree module is not, it should not give undefined symbol errors during load (in fact, there's no reason it shouldn't work).  The biggest problem areas seemed to be DEBUG_THREADS and DEBUG_CHANNEL_LOCKS.
> 
> Is this the right way to go about this?
> 
> 
> Note: Many of the functions in lock.h had to be moved into a .c file (main/lock.c).  They were defined as static inline, which defeats much of the purpose.  A module *would* have the information about where a lock was obtained, but since it was including lock.h without DEBUG_THREADS enabled, it wouldn't bother logging it - even though Asterisk would really like it to.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/astobj2.h 249755 
>   /branches/1.4/include/asterisk/lock.h 249755 
>   /branches/1.4/main/Makefile 249755 
>   /branches/1.4/main/astobj2.c 249755 
>   /branches/1.4/main/channel.c 249755 
>   /branches/1.4/main/lock.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/508/diff
> 
> 
> Testing
> -------
> 
> Compile tests with/without DEBUG_THREADS and DEBUG_CHANNEL_LOCKS work great.  I've not tried building an external module with different flags yet.  I suspect that one would compile fine, but may not act properly, partially because of the note above.
> 
> 
> Thanks,
> 
> qwell
> 
>