Travis Cross wrote:

>> * TLS and IPv6 can be turned off individually. To be fair though, I
>>   realized that the original tcp_dispatcher:get_listenerspecs/0 didn't
>>   check for IPv6 being disabled either =).
> I thought about this, but in some ways it seemed that enable_v6 and
> tls_disable_server were redundant anyway when one is explicitly
> configuring the service / bind address tuples.  Might it just be
> simpler if enable_v6 and tls_disable_server were removed, and users
> could enable ipv6 with:
>   {listen_on, [{udp6,"",5060},
>                {tcp6,"",5060}]},
> or disable tls just by leaving it out of the listen_on list?  In any
> case, doing this would reduce the possibility of specifying
> conflicting or ambiguous configuration parameters.

Good thinking - the syntax would be {udp6, "::", 5060} though. What I'm 
really missing here is

   1) ability to listen to the same port(s) for all my IP addresses, be
      they IPv4 or IPv6
   2) sensible defaults

The sensible defaults I think should be as it is today, listen on tcp 
and udp port 5060, and tls port 5061. I was going to say 'for both IPv4 
and IPv6' but that wasn't actually the case today I noticed =). IPv6 _if 
possible_ would be a good default.

>> * TLS is not included in siphost:default_listen_list/0.
> Good catch.  TLS should indeed go in the default listen list.
>> * With your patch, it is possible to listen to multiple different ports
>>   of a specific protocol. While this is a good thing in itself, it comes
>>   with a whole battery of ... interesting things to look into. When
>>   receiving a request where we decide we need to put the port number in
>>   the Via header, we should probably take care and put the exact port
>>   number we received the request over in there, and likewise when we are
>>   routing responses we can no longer just look for a connection to the
>>   right remote IP:port, but must make sure that it is also with the
>>   right local IP:port.
> I did have a sense that something like this might be the case,
> though I wasn't quite sure where to start looking.  I'll certainly
> dig a bit deeper now to see how this might be addressed.  My feeling
> is that this could be fixed in a post-merge revision, as it doesn't
> break existing functionality, and users could by warned of this in
> the meantime.

It introduces a great way for users to shoot themselves in the foot, so 
I think the cautious way forward with your patch would be to only accept 
one port per protocol. This means only one port per tcp+tcp6 and 
udp+udp6 and so on. Also, I think there is a requirement in RFC3261 to 
listen to the same ports for UDP and TCP, but I'm not 100% on this. 
Haven't looked.

>> Also, when contributing code it is very much appreciated if you really
>> _really_ try to keep to the same coding style. With your code, the
>> differences are mostly that you don't put spaces after commas and that
>> you format your 'case' statements different than me in a number of
>> places. I know it sucks to have to think about coding style when
>> contributing, but it works the other way around too, when I'm
>> contributing to someone elses project ;)
> That's certainly fair.  When I was otherwise ready to post my
> patches, I went back through and cleaned up the documentation and
> formatting to better match the ambient style.  I still missed some
> elements apparently ;)  I'll go back through and clean this up further.

Thanks for not getting discouraged by this - I certainly didn't want you 
to =).

> As Erlang is growing in
> popularity and use, I'm surprised that this limitation/bug has not
> yet become a major issue [1].


> Even if inet:getiflist/0 and inet:ifget/2 worked appropriately [2],
> I believe that the capability to listen on service/address/port
> tuples selectively is a necessary and sound addition to YXA [3].


Yxa-devel mailing list

Reply via email to