[PATCH] libads: Fix fallback logic when finding a domain controller

Jeremy Allison jra at samba.org
Wed Jun 10 15:51:56 MDT 2015


On Tue, Jun 09, 2015 at 10:19:13PM +0300, Uri Simchoni wrote:
> Oops once again with the patch...
> 
> On Tue, Jun 9, 2015 at 10:18 PM, Uri Simchoni <urisimchoni at gmail.com> wrote:
> > This is a patch to fix bug 11321.
> >
> > Basically when finding a domain controller, the method is to resolve
> > the IP address of candidate servers, and then do an ldap ping until a
> > suitable server answers.
> >
> > In case of failure, there's fallback from DNS lookup to netbios lookup
> > (if netbios is enabled) and then back to site-less DNS lookup. The two
> > problems here are:
> > 1. It makes more sense to try site-less DNS before NetBIOS because the
> > fallback to NetBIOS is not likely to give better results.
> > 2. The NetBIOS fallback screws the site-less fallback (I suppose the
> > "goto considered harmful fellows are sometimes right after all...).
> >
> > This fix extracts the core code that does name resolving+ldap ping
> > into a separate function and then activates this function in up to
> > three modes - site-aware, site-less, and netbios, in that order.

Here are some changes I'd like if that's ok.

1). Use the above text as the commit message. It's really helpful
to understand what you're doing here :-).

2).

+       if (!c_realm)
+               c_realm = "";

should be:

+	if (c_realm == NULL) {
+		c_realm = "";
+	}

to keep Samba coding convention.

3). Second addition of:

+       if (!c_realm) {
+               c_realm = "";
+       }

can be removed (you already ensured
c_realm != NULL above and lp_realm()
cannot return NULL (it's defined
as FN_GLOBAL_CONST_STRING which is

#define FN_GLOBAL_CONST_STRING(fn_name,ptr) \
 const char *lp_ ## fn_name(void) {return(*(const char * const *)(&Globals.ptr) ? *(const char * const *)(&Globals.ptr) : ""

4). In the code changes around:

+       if (*c_realm) {
+               sitename = sitename_fetch(talloc_tos(), c_realm);
+               status = resolve_and_ping(ads, sitename, c_realm, true, c_realm);
+               /* In case we failed to contact one of our closest DC on our
+                * site we
+                * need to try to find another DC, retry with a site-less SRV
+                * DNS query
+                * - Guenther */
+
+               if (!NT_STATUS_IS_OK(status) && sitename) {

Instead of doing the:

+               if (!NT_STATUS_IS_OK(status) && sitename) {

do an early return if we got NT_STATUS_OK (you can always
call TALLOC_FREE on a null pointer, it's safe).

4). Would it be possible to add 2
functions instead of the one
resolve_and_ping() ?

One resolve_and_ping_dns(), and
the other resolve_and_ping_netbios()

and get rid of the 'bool use_dns' parameter ?
I know it's some code duplication, but the
logic here is complex enough that two separate
functions would be much easier to follow.

I'm sorry if this seems needlessly nit-picky,
but your code changes are really good, and
getting better, so I'm kind of raising the
bar on what I expect from you (I hope that's
OK :-).

Cheers,

	Jeremy.


More information about the samba-technical mailing list