[asterisk-dev] [Code Review] IPv6 in Asterisk

Tue Jun 29 08:17:18 CDT 2010

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_iax2.c, lines 12240-12241
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11151#file11151line12240>
> >
> >     Braces around if statements.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_mgcp.c, lines 1779-1780
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11153#file11153line1779>
> >
> >     Same as above.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 3222-3224
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3222>
> >
> >     Space after if, and braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 3233-3235
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3233>
> >
> >     same as above.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 3256-3261
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3256>
> >
> >     Braces for if statements.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 3264-3269
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3264>
> >
> >     same.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 3895-3898
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line3895>
> >
> >     Braces, since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 4369-4370
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line4369>
> >
> >     braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 4941-4942
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line4941>
> >
> >     braces for if statements.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 5001-5003
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line5001>
> >
> >     More braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 5008-5011
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line5008>
> >
> >     same.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 5013-5014
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line5013>
> >
> >     same.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 5019-5020
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line5019>
> >
> >     Braces since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 5022-5023
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line5022>
> >
> >     Space between if and (, plus braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 7058-7060
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line7058>
> >
> >     braces since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 7769
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line7769>
> >
> >     Same, space between if and (

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 8202-8203
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line8202>
> >
> >     Braces since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 8245-8246
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line8245>
> >
> >     Again, braces since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 8262-8263
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line8262>
> >
> >     Same.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 8503-8507
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line8503>
> >
> >     Space between if and (, braces for if statements.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 8511
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line8511>
> >
> >     same, spacing.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 9053
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9053>
> >
> >     spaceing

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 9076
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9076>
> >
> >     spacing

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 9084-9086
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9084>
> >
> >     braces

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 9088-9089
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9088>
> >
> >     Braces, since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 9517-9519
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line9517>
> >
> >     braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10041-10044
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10041>
> >
> >     same, braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10266-10267
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10266>
> >
> >     since we are here, braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10279-10280
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10279>
> >
> >     same

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10291-10292
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10291>
> >
> >     same

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10404-10405
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10404>
> >
> >     same

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10756-10760
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10756>
> >
> >     Braces, since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 10875-10876
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line10875>
> >
> >     same as above.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 11936-11937
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line11936>
> >
> >     braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 12444-12446
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12444>
> >
> >     Braces for if statements.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 12452-12458
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12452>
> >
> >     Braces

Fixed. And oh wow, there was actually a huge bug here!!!

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 12554
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12554>
> >
> >     spacing between if and (

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 12564-12569
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12564>
> >
> >     Braces

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 12714
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12714>
> >
> >     blob

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 12779
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line12779>
> >
> >     blob

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 13503-13504
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line13503>
> >
> >     Braces, since we are here.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 13533
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line13533>
> >
> >     This appears to be a change in behaviour for the manager event 'PeerStatus', was this intentional?
> >     
> >     We have dropped the 'Port' setting.

No, it was a mistake. Reverted to previous behaviour.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 14348
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14348>
> >
> >     spacing between + sign

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 14401-14404
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14401>
> >
> >     Braces.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 14410-14411
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14410>
> >
> >     Same.

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 14442-14444
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14442>
> >
> >     Braces

Fixed.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, line 14941
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line14941>
> >
> >     Again, this is a change in behaviour.  Do we need to be merging host and port?

In this case, yes, it was intentional. This is a cosmetic change only, not an API change like the previous case.

Logically, host and port are two components of an "address", which should be viewed as a single atomic entity. Following this principle makes a lot of things easier.

> On 2010-06-28 15:37:42, pabelanger wrote:
> > /trunk/channels/chan_sip.c, lines 15232-15235
> > <https://reviewboard.asterisk.org/r/743/diff/3/?file=11155#file11155line15232>
> >
> >     Braces since we are here.

Fixed.

- Simon

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

On 2010-06-28 14:29:33, Simon Perreault wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/743/
> -----------------------------------------------------------
> 
> (Updated 2010-06-28 14:29:33)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This is the port of Asterisk to IPv6.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_ooh323.c 272684 
>   /trunk/apps/app_externalivr.c 272684 
>   /trunk/channels/chan_gtalk.c 272684 
>   /trunk/channels/chan_h323.c 272684 
>   /trunk/channels/chan_iax2.c 272684 
>   /trunk/channels/chan_jingle.c 272684 
>   /trunk/channels/chan_mgcp.c 272684 
>   /trunk/channels/chan_multicast_rtp.c 272684 
>   /trunk/channels/chan_sip.c 272684 
>   /trunk/channels/chan_skinny.c 272684 
>   /trunk/channels/chan_unistim.c 272684 
>   /trunk/channels/sip/dialplan_functions.c 272684 
>   /trunk/channels/sip/include/dialog.h 272684 
>   /trunk/channels/sip/include/globals.h 272684 
>   /trunk/channels/sip/include/reqresp_parser.h 272684 
>   /trunk/channels/sip/include/sip.h 272684 
>   /trunk/channels/sip/reqresp_parser.c 272684 
>   /trunk/include/asterisk/acl.h 272684 
>   /trunk/include/asterisk/config.h 272684 
>   /trunk/include/asterisk/dnsmgr.h 272684 
>   /trunk/include/asterisk/netsock2.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 272684 
>   /trunk/include/asterisk/tcptls.h 272684 
>   /trunk/main/acl.c 272684 
>   /trunk/main/app.c 272684 
>   /trunk/main/config.c 272684 
>   /trunk/main/dnsmgr.c 272684 
>   /trunk/main/http.c 272684 
>   /trunk/main/manager.c 272684 
>   /trunk/main/netsock2.c PRE-CREATION 
>   /trunk/main/rtp_engine.c 272684 
>   /trunk/main/tcptls.c 272684 
>   /trunk/res/res_rtp_asterisk.c 272684 
>   /trunk/res/res_rtp_multicast.c 272684 
> 
> Diff: https://reviewboard.asterisk.org/r/743/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Simon
> 
>