[asterisk-dev] [Code Review] Make reloading cdr_pgsql.so not screw up catastrophically.

[rmudgett]

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



/branches/1.8/cdr/cdr_pgsql.c
<https://reviewboard.asterisk.org/r/1711/#comment9946>

    Swap these two lines.
    
    LENGTHEN_BUF1 has an error return that will do the unlock that was just done.



/branches/1.8/cdr/cdr_pgsql.c
<https://reviewboard.asterisk.org/r/1711/#comment9942>

    You should delay unregistering the module until after you have determined if the config has changed or not.
    
    You should change the comment to a block comment in the then clause rather than as a tail comment.
    
    Tail comments need to be very short since there is usually not much room on the end of the line.



/branches/1.8/cdr/cdr_pgsql.c
<https://reviewboard.asterisk.org/r/1711/#comment9943>

    Same here about the tail comment.
    
    Also this does not need to be conditional on a reload.  If it is the initial load it is an effective noop.



/branches/1.8/cdr/cdr_pgsql.c
<https://reviewboard.asterisk.org/r/1711/#comment9945>

    Put this in a destroy function as the unload_module does the same thing.



/branches/1.8/cdr/cdr_pgsql.c
<https://reviewboard.asterisk.org/r/1711/#comment9947>

    For safety, the WRLOCK needs to be obtained here.


- rmudgett


On Feb. 2, 2012, 3:34 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1711/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2012, 3:34 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Without this patch, attempting cli command:
> module reload cdr_pgsql.so
> 
> will cause the columns list to get doubled up (because they aren't cleared) which will make insertions fail since the insert statement will have all of its elements repeated.
> 
> This patch does two things.  Well, three.  First it unregisters cdr_pgsql from cdr.c so that cdr logs won't try to use this cdr storage method while the reload process is happening (re-registration occurs at the end of the reload).  Second, it clears the columns list during reload so that it doesn't end up doubling up the entires.  As an irrelevant third, it updates the copyright status in the documentation to 2012 from 2006 or so.
> 
> 
> This addresses bug ASTERISK-19216.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19216
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/cdr/cdr_pgsql.c 353844 
> 
> Diff: https://reviewboard.asterisk.org/r/1711/diff
> 
> 
> Testing
> -------
> 
> Tested with and without patch. With patch, the CDRs get logged correctly after reloading without the errors mentioned in the bug report and also the reload doesn't spit out a "this cdr backend has already been registered in cdr.c" (paraphrasing) type error.
> 
> 
> Thanks,
> 
> jrose
> 
>

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