Fredrik Thulin wrote:
> Travis Cross wrote:
>> Fredrik Thulin wrote:
>>> I think the 'what-to-listen-on' should be made more generic. I have
>>> had thoughts along the lines of
>>>
>>> {listen_on, ["tls:127.0.0.1:5061",
>>
>> I've implemented something quite similar to your proposal. The
>> patch against svn trunk is attached.
>>
>> To avoid extra parsing and making guesses, I kept a slightly more
>> code-as-data format:
>>
>> {myips, ["192.168.1.100",
>> {listen_on, [{tcp,"0.0.0.0",5060},
>>
>> Since my machines are not configured for ipv6, the ipv6 part of the
>> code has not received end-to-end testing.
>>
>> If myips is not explicitly configured, the autodetection code still
>> kicks in.
>>
>> I attempted to cleanly delimit throughout YXA the code that asks
>> "what IPs are on this box?" from the code that needs to know "what
>> listen specification should I use?", but you'll clearly want to look
>> this over.
>
> I've had a closer look now - actually tried it out. While I believe you
> have made a number of very good choices, I'm afraid that there are a few
> important things you have overlooked, or more probably wasn't aware of.
>
Thanks for looking it over. As some of the changes do indeed raise
interesting questions, I was hoping for feedback.
> * 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,"0.0.0.0",5060},
{tcp6,"0.0.0.0",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.
> * 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.
> 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.
> To sum it up: I was going to include your patch in a snapshot I'm
> preparing, but unfortunately it needs a bit more time than I have at my
> disposal at the moment, so it will have to wait until you, me or someone
> else finishes the work.
I'll see what I can do here, and will likely send along a patch shortly.
> In the mean time I'd suggest that you post on
> erlang-questions about the root to your problem, which I'd say is an
> Erlang/OTP issue, even if inet:getiflist/0 is undocumented and unsupported.
I may do this. I've looked through the Erlang/OTP sources to get a
sense of what it might take to fix the problem. I usually like to
have a patch accompanying a problem report. 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].
Cheers,
-- Travis
Notes:
[1] It may have, of course, and I just haven't seen the thread yet.
[2] inet:ifget/2 needs to be changed to return multiple addresses.
I wonder how much current code would be broken by doing this.
[3] Other SIP proxies such as OpenSER have this functionality.
There's no reason YXA should concede this ground.
_______________________________________________
Yxa-devel mailing list
Yxa-devel@lists.su.se
https://lists.su.se/mailman/listinfo/yxa-devel
