[SSSD] [PATCHES] ad srv: prefer servers that are in the same domain as client

Pavel Březina pbrezina at redhat.com
Mon Aug 26 10:08:56 UTC 2013


On 08/23/2013 07:39 PM, Jakub Hrozek wrote:
> On Fri, Aug 23, 2013 at 07:36:22PM +0200, Pavel Březina wrote:
>> On 08/23/2013 05:44 PM, Jakub Hrozek wrote:
>>> On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
>>>> On 08/08/2013 03:32 AM, Simo Sorce wrote:
>>>>> On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
>>>>> [..]
>>>>> nitpicking some more given Lucas nacked.
>>>>>
>>>>>>> +static errno_t ad_sort_servers(struct ares_srv_reply **list, void *pvt)
>>>>>>> +{
>>>>>>> +    struct ad_srv_plugin_state *state = NULL;
>>>>>>> +    struct ares_srv_reply dummy[3] = {{NULL, NULL, 0, 0, 0}};
>>>>>
>>>>> I think this can be done by simply writing:
>>>>>
>>>>> 	struct ares_srv_reply dummy[3] = { 0 };
>>>>>
>>>>>>> +    struct ares_srv_reply *head = &dummy[0];
>>>>>>> +    struct ares_srv_reply *tail = &dummy[0];
>>>>>>> +    struct ares_srv_reply *in_head = &dummy[1];
>>>>>>> +    struct ares_srv_reply *in_tail = &dummy[1];
>>>>>>> +    struct ares_srv_reply *out_head = &dummy[2];
>>>>>>> +    struct ares_srv_reply *out_tail = &dummy[2];
>>>>>>> +    struct ares_srv_reply *cur = NULL;
>>>>>>> +    struct ares_srv_reply *next = NULL;
>>>>>>> +    const char *domain = NULL;
>>>>>>> +    errno_t ret;
>>>>>>> +
>>>>>>> +    if (list == NULL) {
>>>>>>> +        ret = EINVAL;
>>>>>>> +        goto done;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (*list == NULL || (*list)->next == NULL) {
>>>>>>> +        ret = EOK;
>>>>>>> +        goto done;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    state = talloc_get_type(pvt, struct ad_srv_plugin_state);
>>>>>>> +    domain = state->discovery_domain;
>>>>>>> +
>>>>>>> +    /* first sort by rfc2782 */
>>>>>>> +    ret = resolv_sort_srv_reply(list);
>>>>>>> +    if (ret != EOK) {
>>>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("resolv_sort_srv_reply() failed "
>>>>>>> +                                    "[%d]: %s\n", ret, strerror(ret)));
>>>>>>> +        goto done;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* when several servers share priority, prefer the one that is located
>>>>>>> +     * in the same domain as client (e.g. child domain instead of forest
>>>>>>> +     * root) */
>>>>>>> +
>>>>>>> +    next = *list;
>>>>>>> +    do {
>>>>>>> +        cur = next;
>>>>>>> +        next = cur->next;
>>>>>>> +
>>>>>>> +        if (is_host_in_domain(cur->host, domain)) {
>>>>>>> +            in_tail->next = cur;
>>>>>>> +            in_tail = in_tail->next;
>>>>>>> +            in_tail->next = NULL;
>>>>>>> +        } else {
>>>>>>> +            out_tail->next = cur;
>>>>>>> +            out_tail = out_tail->next;
>>>>>>> +            out_tail->next = NULL;
>>>>>>> +        }
>>>>>>> +
>>>>>>> +        if (next == NULL || cur->priority != next->priority) {
>>>>>>> +            /* priority has changed or we have reached the end of the srv list,
>>>>>>> +             * we will merge the lists */
>>>>>>> +            APPEND_ARES_LIST(tail, in_head, in_tail);
>>>>>>> +            APPEND_ARES_LIST(tail, out_head, out_tail);
>>>>>>> +        }
>>>>>>> +    } while (next != NULL);
>>>>>>> +
>>>>>>> +    *list = head->next;
>>>>>>> +
>>>>>>> +    ret = EOK;
>>>>>>> +
>>>>>>> +done:
>>>>>>> +    return ret;
>>>>>>> +}
>>>>>>
>>>>>> I am sorry, but this linked list sorting is not very readable.
>>>>>> There is a lot of "struct ares_srv_reply" pointers:
>>>>>>      head, tail, in_head, in_tail, out_head, out_tail, cur, next
>>>>>> I tried to visualize this sorting with pencil and paper, but
>>>>>> it was a real mess.
>>>>>>
>>>>>> I would prefer to:
>>>>>>    either: use similar approach like in reply_priority_sort
>>>>>>            from file src/resolv/async_resolv.c (merger-sort)
>>>>>>    or: don't change this function, but unit test should be written for this
>>>>>>        function
>>>>>
>>>>> Why not simply make this a doubly linked list and use the functions in
>>>>> src/util/dlinklist.h ?
>>>>
>>>> I'm working with struct ares_srv_reply that comes from ares api.
>>>>
>>>>> Or if you really have to use single linked, maybe add a slinklist.h file
>>>>> in util/ that does things as much as possible like dlinklist.h so that
>>>>> we use the same style.
>>>>
>>>> Done.
>>>>
>>>>
>>>>
>>>
>>> I was wondering if we make the ordering plugin a property of the SRV
>>> lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
>>> a new function? In general, I think that the sorting would be only used
>>> by the same provider as the lookup plugin, so it would be nice if we
>>> could avoid new parameters for a generic function and keep the API
>>> clean.
>>
>> Hmm, then I guess I can move the whole functionality into the plugin
>> itself. Instead of sorting ares result in fo_discover_srv*, we can
>> sort fo_server_info in the AD plugin. It will require some changes I
>> wanted to avoid, but its doable.
>>
>> Does this sound right?
>
> I would prefer this way but you know the failover code better now
> probably. So what are the changes you wanted to avoid?

Well I either will have to change fo_server_info from array to dlist or 
rewrite the whole sorting function. At the moment, I prefer the latter 
one. :-)

But I have another idea: how about we make it part of default SRV 
sorting? Preferring those servers that are in the same DNS domain makes 
sense also for LDAP and IPA provider.




More information about the sssd-devel mailing list