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

[jrose]

> On Feb. 3, 2012, 2:26 p.m., rmudgett wrote:
> > /branches/1.8/cdr/cdr_pgsql.c, lines 663-668
> > <https://reviewboard.asterisk.org/r/1711/diff/3/?file=23883#file23883line663>
> >
> >     If you move the ast_cdr_register to load_module, you can simplify this code since only loading the module needs to register.
> 
> jrose wrote:
>     I think the simplifications here are actually a lot more costly in complications below. ast_cdr_register has its own failure conditions and I'd basically have to come up with a new return value to indicate that I actually would have performed ast_cdr_register from this function which would just have to be evaluated in the load_module function anyway.
> 
> rmudgett wrote:
>     Here:
>     config_module()
>     {
>     ....
>     
>     	ast_mutex_unlock(&pgsql_lock);
>     	return 0;
>     }
>     
>     static int load_module(void)
>     {
>     	if (config_module(0)) {
>     		return AST_MODULE_LOAD_DECLINE;
>     	}
>     	return ast_cdr_register(name, ast_module_info->description, pgsql_log)
>     		? AST_MODULE_LOAD_DECLINE : 0;
>     }
>

Well, I'm just bothered right now about what happens in the earlier return zero from this line 407.  As far as I can tell, this shouldn't result in a register, but when employing this change pgsql does get registered. This doesn't change a whole lot as far as running behavior goes, but the pgsql_log function will be ran anyway, which will mean it takes a lock, fail a condition to enter the meat of the function, and then immediately release the lock.

And that seems undesirable to me.


- jrose


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


On Feb. 3, 2012, 4:06 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1711/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2012, 4:06 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 353998 
> 
> 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/20120206/1cff0f23/attachment.htm>