[SSSD] [PATCHES] ad srv: prefer servers that are in the same domain as client
Jakub Hrozek
jhrozek at redhat.com
Fri Aug 23 17:39:00 UTC 2013
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?
More information about the sssd-devel
mailing list