[asterisk-dev] [Code Review] fix excess warnings after r1395 (and disallow empty names always)

[schmidts]

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

Ship it!


looks fine to me. only some small stuff to fix.



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1560/#comment8839>

    wouldnt be a break a little bit cleaner than a return from inside a loop?



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1560/#comment8840>

    for me this would make more sense turning this if around and move the realtime call to the else part.
    if we dont have a name and cant get it from *var then destroy var, throw a warning.... Else just go ahead and do the realtime lookup.
    



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1560/#comment8841>

    i have tested this row and it works fine :D


best regards

- schmidts


On Nov. 2, 2011, 3:16 p.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1560/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2011, 3:16 p.m.)
> 
> 
> Review request for Asterisk Developers, Terry Wilson and schmidts.
> 
> 
> Summary
> -------
> 
> r1395 broke two things:
> - It added warnings for situations that shouldn't get a warning. (Bad placement of the warning.) [schmidts found this]
> - It was allowed to have an empty sippeer name. (No check after get_name_from_variable().)
> 
> However, both before and after r1395, the following was broken:
> - For port-insecure matches, it was allowed to have an empty peer name. (Because of the use of ast_load_realtime_multientry.)
> - Realtime lookups for name NULL were attempted with undefined behaviour (res_config_sqlite returns a warning "1 parameter and 1 value at least required" and no results)
> 
> This patch does the following:
> - Fix the warning to only be emitted when there actually is an empty peername found. (Both in case of sipregs and sippeers.)
> - Disallow the empty peername always. Even for port-insecure matches.
> 
> Notes:
> - For sippeer matching: if name is empty, the match is used and discarded at the end with a warning. (The behaviour as before r1395.)
> - For the port-insecure matching of sipregs: if name is empty, the match is discarded and lookup continues. (This wasn't available before r1395.)
> - For other matching of sipregs: if name is empty, a warning is emitted, the match is discarded and lookup continues. (This was undefined before r1395; the NULL-name was refused, the empty-name was allowed.)
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 342927 
> 
> Diff: https://reviewboard.asterisk.org/r/1560/diff
> 
> 
> Testing
> -------
> 
> I've been staring at this for a couple of hours now. It should be better than it was.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

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