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.