On Thu, 2009-12-10 at 12:05 -0500, Dale Worley wrote:
> This patch changes the behavior of SipSrvLookup::servers() without
> documenting that change in the description of servers(), although it is
> documented in ::setOwnHostname().
Good catch... it should be documented in SipSrvLookup::servers as well.
> But I'm wondering about the exact effect: Do we want to base this on
> the hostname or do we want to base it on the address? It seems to me
> that the latter is less ambiguous and more universal (as it would take
> effect regardless of the hostname used).
I did the selection/sort-weighting on names rather than addresses
deliberately... it seemed to me to be more in line with the conceptual
structure of the SRV mechanism. An SRV record points to a name, not to
an IP address. That name may have many addresses, and an address may
have many names. Using the name to bias the weighting seemed less
likely to cause problems than trying to do something at the address
level (it was also _much_ easier to implement).
The goal here is to prefer (when all else is equal) to loopback a packet
rather than to put it on the wire - there's at least some gain in
performance, and it certainly makes debugging easier. This change will
do that, as long as the host name in the SRV record matches the one
passed to setOwnHostname - and if it doesn't, then this change has no
effect on the selection (which seems to be a nice fail-safe quality).
> Nits:
>
> The comment needs to be fixed here:
>
> --- a/sipXtackLib/src/net/SipSrvLookup.cpp
> +++ b/sipXtackLib/src/net/SipSrvLookup.cpp
> @@ -241,6 +241,9 @@ int SipSrvLookup::options[OptionCodeLast+1] = {
> };
>
> /// Sets the timeout parameter for DNS SRV queries. Default is 3
> +UtlString SipSrvLookup::mOwnHostname;
> +
> +/// Sets the timeout parameter for DNS SRV queries. Default is 3
> int SipSrvLookup::mTimeout = 3;
>
> Are you sure you want to include this?
No... the Makefile change was just for testing convenience - I'll undo
that before I check anything in.
> --- a/sipXtackLib/src/test/Makefile.am
> +++ b/sipXtackLib/src/test/Makefile.am
> @@ -1,6 +1,6 @@
> ## All tests under this GNU variable should run relatively quickly
> ## and of course require no setup
> -TESTS = testsuite
> +TESTS = sandbox
>
> check_PROGRAMS = testsuite sandbox
>
> @@ -75,4 +75,4 @@ EXTRA_DIST = \
>
> sandbox_LDADD = $(testsuite_LDADD)
> sandbox_CXXFLAGS = $(testsuite_CXXFLAGS)
> -sandbox_SOURCES =
> +sandbox_SOURCES = net/SipSrvLookupTest.cpp
>
> And there's a formatting problem here:
> @@ -99,3 +113,7 @@ test6b.example.com 1D IN A 2.1.6.1
> test6udp.example.com 1D IN A 2.1.6.0
> test6tcp.example.com 1D IN A 2.1.6.1
> test7.example.com 1D IN A 2.1.7.0
> +
> +testself.example.com 1D IN A 2.1.8.1
> +testother2.example.com 1D IN A 2.1.8.2
> +testother3.example.com 1D IN A 2.1.8.3
Thanks for the careful review.
_______________________________________________
sipx-dev mailing list [email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
sipXecs IP PBX -- http://www.sipfoundry.org/
