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

[wdoekes]

> On Nov. 2, 2011, 4:06 p.m., schmidts wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4717-4720
> > <https://reviewboard.asterisk.org/r/1560/diff/1/?file=21575#file21575line4717>
> >
> >     wouldnt be a break a little bit cleaner than a return from inside a loop?

It is. Fixed.


> On Nov. 2, 2011, 4:06 p.m., schmidts wrote:
> > /branches/1.8/channels/chan_sip.c, line 4832
> > <https://reviewboard.asterisk.org/r/1560/diff/1/?file=21575#file21575line4832>
> >
> >     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.
> >

Ok, but I don't like return 0, return 1, return 0 either. So I changed the order some more.


- wdoekes


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


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/3561ddf7/attachment.htm>