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

Uri Simchoni urisimchoni at gmail.com
Thu Jun 11 05:37:40 MDT 2015


Hopefully I captured everything - here's V2, split into two commits:

1/2 - same as V1 1/1, but with a modified commit message and hopefully
capturing the coding-style comments.
2/2 - further splitting the new resolve_and_ping into dns and netbios
variants - I figured this belongs on a separate commit because
extracting code into a function is hard enough to review, let alone
into two functions.

Thanks,
Uri.

On Thu, Jun 11, 2015 at 12:51 AM, Jeremy Allison <jra at samba.org> wrote:
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: site-fb2.patch
Type: application/octet-stream
Size: 15974 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150611/2eb2be1c/attachment.obj>


More information about the samba-technical mailing list