[asterisk-dev] [Code Review] va_args cleanup in res_config_odbc, res_config_curl, utils, and also res_config_pgsql

[wdoekes]

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


Thanks for picking this up. You can trim the patch size (and code readability) by a bit if you move some stuff around though:


/branches/1.8/res/res_config_odbc.c
<https://reviewboard.asterisk.org/r/1848/#comment10902>

    Locality. You're not using cps.ap nor aq until a bit down. Consider moving the va_copy's to just before you need them. That'll save you lots of cleanup. 
    
    E.g.:
    
    va_copy(cps.ap, ap);
    stmt = bla(&cps)
    va_end(cps.ap);


- wdoekes


On April 5, 2012, 12:53 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1848/
> -----------------------------------------------------------
> 
> (Updated April 5, 2012, 12:53 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, wdoekes, and Matt Jordan.
> 
> 
> Summary
> -------
> 
> A number of va_copy operations weren't matched with a corresponding va_end in res_config_odbc. Also, there was a potential for va_end to be invoked twice on the same va_arg in utils, which would mean invoking va_end on an undefined variable... which is bad.
> 
> va_end is removed from various functions in config_pgsql and config_curl since they aren't making their own copy.  The invokers of those functions are responsible for calling va_end on them.
> 
> 
> This addresses bug ASTERISK-19451.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19451
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/utils.c 361291 
>   /branches/1.8/res/res_config_curl.c 361291 
>   /branches/1.8/res/res_config_odbc.c 361291 
>   /branches/1.8/res/res_config_pgsql.c 361291 
> 
> Diff: https://reviewboard.asterisk.org/r/1848/diff
> 
> 
> Testing
> -------
> 
> None to speak of.  It compiles, but I'm really not sure how to check for va_arg leaks and such.  I'm not too worried about res_config_odbc and utils since those changes are pretty self-contained. The other changes should be fine too since the paramater ap in res_config_odbc isn't ended in any of the adapter functions there. I'm a little wary though that they should be copying to a new va_arg list instead like res_config_odbc does.
> 
> 
> Thanks,
> 
> jrose
> 
>

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