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.