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.