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

[Simon Perreault]

> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/include/asterisk/netsock2.h, lines 97-102
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11107#file11107line97>
> >
> >     Any reason this couldn't simply be:
> >     memcpy(dst, src, sizeof(*dst))?
> >     It's not like we're doing any byte ordering conversions here.

It's an optimization. src->len will often be much smaller than sizeof(*dst).


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/include/asterisk/netsock2.h, lines 128-130
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11107#file11107line128>
> >
> >     Per convention, 1 << 0, 1 << 1, 1 << 2.  This prevents errors in the future, when getting into the higher ranges of bitwise operations.

Agreed.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/config.c, line 2401
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11112#file11112line2401>
> >
> >     Red blob.

Fixed, thanks.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/config.c, line 2405
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11112#file11112line2405>
> >
> >     Is this a TODO for the future or something that was merely missed?

PARSE_INADDR is still used by the STUN subsystem, which is still IPv4-only. When everything has been converted then we can remove it. Therefore it is a TODO for the future, after this patch.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/http.c, lines 1004-1008
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11114#file11114line1004>
> >
> >     Convention:  should these be initialized at declaration time?

Sure, no problem.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/netsock2.c, lines 78-80
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11116#file11116line78>
> >
> >     This appears to be an opencode of the ast_str_* dynamic string implementation.  We already have ast_str_thread_get() and it probably should be used here.

Agreed. Will be fixed.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/netsock2.c, line 104
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11116#file11116line104>
> >
> >     ast_copy_string, please

Agreed.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/netsock2.c, line 111
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11116#file11116line111>
> >
> >     ast_copy_string

Agreed.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/netsock2.c, lines 340-345
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11116#file11116line340>
> >
> >     The number of casts in this module seems to be a good case for putting a union within struct ast_sockaddr.  Casts can be quite error-prone.

See discussion above.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/main/http.c, lines 319-323
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11114#file11114line319>
> >
> >     Instead of doing this conversion, does it make more sense merely to convert directly to the output format?

Sure, no problem.


> On 2010-06-25 22:41:07, Tilghman Lesher wrote:
> > /trunk/include/asterisk/netsock2.h, lines 53-56
> > <https://reviewboard.asterisk.org/r/743/diff/2/?file=11107#file11107line53>
> >
> >     I think this might actually be better as a union between the two sockaddr types, so that we could avoid having to do conversions all over the place and instead just do the conversions directly.  If you disagree, could you comment on this a bit further?

First: Even if we prefer explicit casts, we will happily change the code if you ask us to. We are absolutely not adamant about this.

Now, the discussion:

To be clear, we are talking about replacing this:

struct ast_sockaddr {
	struct sockaddr_storage ss;
	socklen_t len;
};

with this:

struct ast_sockaddr {
	union {
		struct sockaddr_storage	ss;
		struct sockaddr_in	sin;
		struct sockaddr_in6	sin6;
	} s;
	socklen_t len;
};

Casts to (struct sockaddr_in *) and (struct sockaddr_in6 *) could be made implicit with the use of the union. There are 8 instances of such casts in the code right now: 6 in netsock2.c and 2 in chan_sip.c.

We have decided against using a union precisely for the reason you are advocating: they make casts implicit. Because casts are dangerous, we want them to be explicit. We want them to be so ugly that we refrain from using them except when there is absolutely no other way.

Implicit casts hide bugs. This code has a cast in it even if it looks innocent:

addr->s.sin.sin_port = htons(5060);

On the other hand, this code is ugly and highly suspicious, exactly as it should be:

((struct sockaddr_in *)&addr->ss)->sin_port = htons(5060);


- Simon


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


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