[asterisk-dev] [Code Review]: Add config parsing example code to app_skel

[Terry Wilson]

> On April 4, 2012, 4:38 p.m., rmudgett wrote:
> > /trunk/apps/app_skel.c, line 350
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line350>
> >
> >     A process_config lock is needed to serialize loading/reloading configuration requests.

Are you sure? The serialization happens at the apply_config stage in this example. Multiple simultaneous reloads will create new objects and will be swapped atomically.


> On April 4, 2012, 4:38 p.m., rmudgett wrote:
> > /trunk/apps/app_skel.c, lines 158-175
> > <https://reviewboard.asterisk.org/r/1841/diff/1/?file=26992#file26992line158>
> >
> >     With the global_config_transaction lock, you only have config synchronization when updating the configs not when reading them.  Also the global_config_transaction lock is trying to do the job of the rwlock in the ao2 global_config.
> >     
> >     For configuration, you *only* need one position in the ao2_global_obj array.  The other array positions are for *independent* global ao2 objects so they can share a lock rather than declaring another AO2_GLOBAL_OBJ_STATIC.
> >     
> >     Another thing to consider is if the ao2 config objects (skel_global_config, the global pvt_cfg container, and skel_pvt) need to even have a lock.  I don't think they do because they are read only and a config reload replaces them safely with a new object.
> >     
> >     ********
> >     
> >     Since the object of this skeleton code is to show how to safely update the config while a module is still running, any active objects need to always refer to consistent configuration.  Updating the configuration while running is always tricky to get right and there are so many ways to do it.
> >     
> >     Proposal for structure definitions:
> >     
> >     /*!
> >      * Global configuration options.
> >      * Does not have an ao2 lock.
> >      */
> >     struct skel_global_config {
> >       ...global cfg values...
> >     }
> >     
> >     /*!
> >      * Section configuration options that depend upon global options.
> >      * Does not have an ao2 lock.
> >      */
> >     struct skel_section_config {
> >       /*!
> >        * \brief Global config depended upon by the section config.
> >        * \note Holds a reference.
> >        */
> >       struct skel_global_config *global;
> >       ...section cfg values...
> >     }
> >     
> >     /*!
> >      * Active private object that uses section configuration.
> >      *
> >      * Requires an ao2 lock since the active object variables and
> >      * the cfg pointer can change.
> >      */
> >     struct skel_active_pvt {
> >       /*!
> >        * \brief Config used by the active skeleton object
> >        * \note Holds a reference.
> >        * \note Any use of global config values needs to be done through
> >        * this cfg pointer since we are dealing with a section config user.
> >        * \note This cfg pointer will change if the configuration changes
> >        * on a reload.
> >        */
> >       struct skel_section_config *cfg;
> >       ...active object variables...
> >     }
> >     
> >     enum {
> >       /*!
> >        * Global configuration object
> >        *
> >        * Any use of this position should not care about any active
> >        * section configuration.
> >        */
> >       GLOBAL_CFG,
> >       /*! Active struct skel_active_pvt's container */
> >       ACTIVE_OBJECTS,
> >     
> >       NUM_GLOBAL_OBJS
> >     };
> >     
> >     static AO2_GLOBAL_OBJ_STATIC(global_objs, NUM_GLOBAL_OBJS);
> >     
> >

Ok, I've played around with this and I don't think it will work the way I'd like. We want to make it so that the entire config is parsed. If it doesn't parse correctly or *anything* errors out in the process, then *no* changes take place at all.

With having a link to the cfg in the private, you still have all kinds of issues where things happen a little at a time and could fail. One example is after parsing all all of the configs into config structs, if there are new privates that need to be added, both allocating and linking could fail. If this happens, you end up with some of the config applied, and some not. You also run into issues where when accessing the pvt->cfg data, you need to ensure that the function has a reference to the cfg itself so it doesn't disappear. Having the pointer there makes it very easy for someone to "do the wrong thing" and just use it.

I think I've come up with another design where we have a separate container of globals, pvts, and pvt_cfgs (all referenced in the static global ao2 array). The pvts and pvt_cfgs are keyed off of the same "name". This means that anyone accessing the config for a private has to do a find() and get a proper reference to the cfg. Also, when parsing the config, new pvt_cfgs are created and added to a new pvt_cfg container. At the same time, we do find_or_create_pvt() for each thing we create a pvt_cfg for and add those pvts (a mix of references to existing pvts and new pvts) to a new pvt container. If all of that succeeds, then we have a new valid config and we swap out the global, pvt_cfg, and pvt containers, unreffing the old containers. This will leave existing pvts with their non-config-related state intact, while allowing us to atomically apply the configs when they are successfully parsed.

I'm working on example code now.


- Terry


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


On April 2, 2012, 2:50 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1841/
> -----------------------------------------------------------
> 
> (Updated April 2, 2012, 2:50 p.m.)
> 
> 
> Review request for Asterisk Developers, rmudgett and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This patch adds config parsing to app_skel to serve as an example of how to properly write config handling using the ao2 global object API for thread-safe reloads. I also made a couple of whitespace changes even though they technically should have gone in a separate patch, but they didn't get in the way of seeing what changed, etc.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_skel.c 360984 
>   /trunk/configs/app_skel.conf.sample PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1841/diff
> 
> 
> Testing
> -------
> 
> "skel show config/privates" and "memory show summary app_skel.c/config.c"
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120410/1839eda1/attachment-0001.htm>