On 08/26/2013 12:15 PM, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 12:08:56PM +0200, Pavel Březina wrote:
> 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.
Yes, I don't have a problem with that. As long as priority and weight
are honored.
I have done it AD only after all. Enabling it for all providers required
more changes than expected. I don't think those changes should be done
in this state of development...