[asterisk-dev] [Code Review] 3343: res_pjsip: Enable DNS support.

Matthew Jordan mjordan at digium.com
Thu Mar 13 17:34:38 CDT 2014


On Thu, Mar 13, 2014 at 4:13 PM, Sean Bright <sean.bright at gmail.com> wrote:
> On 3/13/2014 4:42 PM, Paul Belanger wrote:
>
> +1 with Dan.  Comments aside on DNS functionality (I have opinions but
> sitting this one out). Any functionality should be channel agnostic.
> I too am a little concern'd that statement seems to have changed.
>
>
> In order to make this "channel agnostic" you have three (equally bad)
> options:
>
> Replace Asterisk's internal DNS facilities with PJLIB's, creating a
> mandatory dependency on PJSIP.
> Roll a shiny new DNS API into Asterisk that supports all address types
> (multiple results, weighting, etc.).  Bear in mind that PJSIP would not use
> this new API at all, you would still need to create a PJLIB DNS resolver and
> feed it the nameservers to use.
> Use PJLIB's DNS interface if it is available, otherwise fall back to
> Asterisk's current DNS interface.  This means that you are now maintaining
> two separate interfaces and have to throw a layer of abstraction in while
> you're at it.  In fact, by adding an abstraction layer you would force
> res_pjsip to then unwrap and then re-wrap the abstraction just to get at the
> necessary PJLIB data structures.
>
> Frankly, I don't see what all the hubbub is about.  99.9% of users will
> never touch the nameservers configuration option and it will behave exactly
> as if the system resolver was being used.
>

To build on Sean's sentiments:

(1) This is not "new" functionality - in the sense that we have not
written some amazing new functionality in either the core or a
res_pjsip* module and only want to use it one place. PJSIP itself
already provides DNS resolution. We just want to turn it on.

(2) Disregarding obtaining the nameservers, enabling the DNS
resolution as we want it to be used could be all of the following
code:

    pj_dns_resolver *resolver;

    if (!pjsip_endpt_get_resolver(ast_sip_get_pjsip_endpoint())) {
        pjsip_endpt_create_resolver(ast_sip_get_pjsip_endpoint(), &resolver);
        pjsip_endpt_set_resolver(ast_sip_get_pjsip_endpoint(), resolver);
    }

    Josh did more of this because he's doing a Good Job. It's
certainly fine to look at how we get to those lines of code; maybe we
should not allow the user to specify the nameservers. At the end of
the day, however, those few lines of code enable asynchronous DNS,
multiple SRV records, and all the fanciness that comes with them.
Achieving the same functionality in any other channel driver is
thousands of lines of code. The scale of the problem is not the same
across the code base. Often, when people have proposed some generic
new functionality and only implemented it in a single channel driver,
we do push for its usage everywhere when the level of effort is the
same across all channel drivers: that isn't the case here.

(3) Even if this were a general purpose change - and as Sean pointed
out, it is not - many other general purpose frameworks have been
introduced in Asterisk and have not been globally applied to every
channel driver - the Configuration Framework, Sorcery, and others come
to mind. This is for good reason: the act of introducing them into an
existing channel driver would create more problems, i.e., regressions,
than they would solve. Going forward, we attempt to use these
frameworks where possible - but not at the sake of hurting existing
users of Asterisk. This is clearly one of those cases: as I pointed
out, implementing the kind of DNS resolution this provides (assuming
we had something that provided it) in chan_sip or chan_iax2 is a gut
and rewrite of those channel drivers. There is _no_ way such an
implementation wouldn't be a substantial risk to users of those
drivers.

(4) Applying a new feature everywhere is not a policy [1]. It *is* a
guideline (of a type which until recently, wasn't even written down
[2]). Policies are great; guidelines are great. They should be
followed - but not to the overall detriment of the project. Adhering
to the letter of the law while ignoring the spirit is not something
I'll ever advocate.

[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines
[2] https://wiki.asterisk.org/wiki/display/AST/Code+Review+Checklist

-- 
Matthew Jordan
Digium, Inc. | Engineering Manager
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org



More information about the asterisk-dev mailing list