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.