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

[rmudgett]

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



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10893>

    I used AST_RWLIST_HEAD_STATIC() as a model for the AO2_GLOBAL_OBJ_STATIC() definition.  It needs the static keyword to precede it whereas AST_MUTEX_DEFINE_STATIC() has the static keyword built into it.



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10894>

    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);
    
    



/trunk/apps/app_skel.c
<https://reviewboard.asterisk.org/r/1841/#comment10895>

    A process_config lock is needed to serialize loading/reloading configuration requests.


- rmudgett


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/20120404/3fa99a84/attachment-0001.htm>