[asterisk-dev] [Code Review]: Use new ao2_global_obj API to clean up udptl.c config parsing/reloading

[Terry Wilson]

> On April 5, 2012, 2:56 p.m., Matt Jordan wrote:
> > /trunk/main/udptl.c, lines 87-88
> > <https://reviewboard.asterisk.org/r/1840/diff/1/?file=26989#file26989line87>
> >
> >     This is a bit picky and possibly not worth the effort, but the debug settings should theoretically probably be contained in udptl_global_options as well.
> >     
> >     When a CLI command is used to turn debug on/off, its operating on a separate thread then whatever is going to actually use the debug flags.  This puts us in a condition where the udptldebug flag could be enabled, but we have not yet fully initialized udptldebugaddr.
> >     
> >     One way of doing this would be to obtain a reference to the current config object in the CLI command, lock it, update the values, and unlock it.  Since these values change on a thread other then a reload, we'd have to lock the config object anytime we wanted to read values from it as well (or at least the debug values).
> >     
> >     Alternatively, we could do a clone and swap - grab the config object in the CLI command, make a copy of it, populate it with the new debug values, and swap it with the new one.  That'd be a lot less locking and less impact on the rest of the code.

I would much rather handle non-config-related state in a separate object. In my opinion, config-holding objects should only be written to by the config code (which will generally be protected by a module-level "reload lock"). If we only have one non-config-related thing that needs to be protected by a lock, making every use of the config object require locking seems like making the default case too complex.

I also don't like clone/copy functions very much since you have to know how to copy every single type stored in the struct and it is always a pain to keep up with. People add fields and forget to update the copy/clone functions, etc. I really don't want to require anyone working with config stuff to have to go down that route if at all possible.


- Terry


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


On April 2, 2012, 2:06 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1840/
> -----------------------------------------------------------
> 
> (Updated April 2, 2012, 2:06 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This patch builds off of https://reviewboard.asterisk.org/r/1455/.
> 
> The differences are:
> 1) Uses the global ao2 object API to store config values and to make reloads thread-safe
> 2) Uses ast_parse_arg() where appropriate
> 3) Updates ast_parse_arg to be able to set defaults to the min/max range value on range errors
> 4) Adds CLI command 'udptl show config'
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/config.h 360980 
>   /trunk/main/config.c 360980 
>   /trunk/main/udptl.c 360980 
> 
> Diff: https://reviewboard.asterisk.org/r/1840/diff
> 
> 
> Testing
> -------
> 
> 1) Used the new 'udptl show config' command to verify that settings looked right on load/reload.
> 2) Did testing with 'memory show summary config.c/udptl.c' with loads/reloads to verify there are no leaks
> 3) Loaded with no config file to verify that default values were used
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120416/ff19a84f/attachment.htm>