[asterisk-dev] Change in asterisk[master]: res_pjsip: Add external PJSIP resolver implementation using ...

[Mark Michelson (Code Review)]

Mark Michelson has posted comments on this change.

Change subject: res_pjsip: Add external PJSIP resolver implementation using core DNS API.
......................................................................


Patch Set 1:

(3 comments)

https://gerrit.asterisk.org/#/c/75/1/main/dns_query_set.c
File main/dns_query_set.c:

Line 112: 	if (query_set->in_progress) {
        : 		return -1;
        : 	}
I suggest having an error message here.

I'd actually go so far as to consider putting an assertion in here because it likely means there is some sort of programmer error occurring if we are attempting to add queries to a set after starting the resolution.


https://gerrit.asterisk.org/#/c/75/1/main/dns_recurring.c
File main/dns_recurring.c:

Line 36: #include "asterisk/vector.h"
       : #include "asterisk/sched.h"
       : #include "asterisk/strings.h"
       : #include "asterisk/dns_core.h"
       : #include "asterisk/dns_recurring.h"
       : #include "asterisk/dns_query_set.h"
On reviewboard, I asked about why these includes got added, and the the issue was marked as resolved, but they're still included now. Are they needed?


https://gerrit.asterisk.org/#/c/75/1/res/res_pjsip/pjsip_resolver.c
File res/res_pjsip/pjsip_resolver.c:

Line 544: 	if ((type == PJSIP_TRANSPORT_UNSPECIFIED && sip_transport_is_available(PJSIP_TRANSPORT_UDP6)) ||
        : 		sip_transport_is_available(type + PJSIP_TRANSPORT_IPV6)) {
        : 		res |= sip_resolve_add(resolve, host, ns_t_aaaa, ns_c_in, (type == PJSIP_TRANSPORT_UNSPECIFIED ? PJSIP_TRANSPORT_UDP6 : type + PJSIP_TRANSPORT_IPV6), target->addr.port);
        : 	}
        : 
        : 	if ((type == PJSIP_TRANSPORT_UNSPECIFIED && sip_transport_is_available(PJSIP_TRANSPORT_UDP)) ||
        : 		sip_transport_is_available(type)) {
        : 		res |= sip_resolve_add(resolve, host, ns_t_a, ns_c_in, (type == PJSIP_TRANSPORT_UNSPECIFIED ? PJSIP_TRANSPORT_UDP : type), target->addr.port);
        : 	}
After giving RFC 3263 another look, I don't think these A and AAAA lookups should be done here.

Section 4.2 says "If no SRV records were found, the client performs an A or AAAA record lookup of the domain name."

Since you can't know if the SRV lookup(s) provided any records yet, performing this lookup is premature.

Now, I guess you could go ahead and perform this lookup early, but if the SRV lookup(s) provide any results, you can't actually use these A/AAAA records.


-- 
To view, visit https://gerrit.asterisk.org/75
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I56cb03ce4f9d3d600776f36928e0b3e379b5d71e
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes