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?