----- Original Message ----- > From: "Jakub Hrozek" <jhro...@redhat.com> > To: sssd-devel@lists.fedorahosted.org > Sent: Monday, April 8, 2013 10:38:47 PM > Subject: Re: [SSSD] [PATCH] DNS site support 1st wave - generic SRV lookup > plugin > > On Mon, Apr 08, 2013 at 04:51:47PM +0200, Jakub Hrozek wrote: > > On Mon, Apr 08, 2013 at 11:55:15AM +0200, Pavel Březina wrote: > > > On 04/05/2013 01:43 PM, Pavel Březina wrote: > > > >On 04/05/2013 12:41 PM, Pavel Březina wrote: > > > >>On 04/03/2013 11:48 AM, Pavel Březina wrote: > > > >>>On 04/02/2013 03:55 PM, Pavel Březina wrote: > > > >>>>On 03/27/2013 03:00 PM, Jakub Hrozek wrote: > > > >>>>>On Wed, Mar 27, 2013 at 11:22:49AM +0100, Pavel Březina wrote: > > > >>>>>>Later, we can place the plugin architecture inside a module that > > > >>>>>>knows about SSSD and is placed above resolver. You said you want to > > > >>>>>>decouple fail over to fail over -> cache -> resolver. This is nice > > > >>>>>>idea. > > > >>>>>> > > > >>>>>>The plugin architecture can be put inside the cache, because cache > > > >>>>>>is responsible for resolving expired uris and _srv_ and it does not > > > >>>>>>really say that is has to use resolver directly. > > > >>>>> > > > >>>>>OK, this would be the best approach. After more discussion on #sssd > > > >>>>>with Pavel and Simo we agreed that Pavel will carry on with this > > > >>>>>approach for the time being. > > > >>>>> > > > >>>>>The only two changes in these patches would be better namespacing of > > > >>>>>the functions used and unifying the resolver initialization with > > > >>>>>what > > > >>>>>I'm working on wrt dynamic DNS updates. > > > >>>>> > > > >>>>>In the next release, we will create the additional layer and move > > > >>>>>the plugins there. I moved https://fedorahosted.org/sssd/ticket/1083 > > > >>>>>which is tracking that effort to NEEDS_TRIAGE so we can put it into > > > >>>>>an appropriate milestone tomorrow. > > > >>>> > > > >>>>Hi, > > > >>>>I'm sending new patch set. > > > >>>> > > > >>>>I created async_resolv_utils.c and put there functions for domain > > > >>>>detection and for resolving SRV with fallback discovery domains. > > > >>>>This is > > > >>>>similar to resolve_srv_send and resolve_get_domain_send from the > > > >>>>first > > > >>>>patchset. > > > >>>> > > > >>>>resolve_srv_send() was renamed to fo_discover_srv_send() and it wraps > > > >>>>resolv_discover_srv_send() so that it returns fo_server_info instead > > > >>>>of > > > >>>>ares. > > > >>>> > > > >>>>The plugin was renamed to fo_resolve_srv_dns_send(). It is a little > > > >>>>bit > > > >>>>more complicated since it performs domain detection (originally in > > > >>>>resolve_srv_send()) if the domain is not provided from fail over. > > > >>>> > > > >>>>Hostname is passed via plugin context. It is currently retrieved via > > > >>>>gethostname() so the current behaviour does not change. Later, when > > > >>>>the > > > >>>>plugin initialization will be moved to provider code, the provider > > > >>>>will > > > >>>>decide what hostname will be used (ipa_hostname for example). > > > >>> > > > >>>Rebased on top of latest be_res patches. > > > >> > > > >>When working on IPA plugin I've done some changes to this code. Since > > > >>it > > > >>is still review pending, I have squashed the changes to this patch set. > > > >> > > > >>1. all SRV plugin related things are in a separate header file named > > > >>fail_over_srv.h. This way we don't have to #include the whole fail over > > > >>when developing new plugin. The only thing which remained in > > > >>fail_over.h > > > >>is fo_set_srv_lookup_plugin(), because it should remain hidden for > > > >>providers, see #2 > > > >> > > > >>2. be_fo_set_srv_lookup_plugin() was added, to keep the fail over > > > >>opaque > > > >>for providers. > > > >> > > > >>3. the original sixth patch (set plugin for all providers) was meant to > > > >>be reverted, once we agreed on further steps. Now it is completely > > > >>rewritten to final solution on which we agreed with Jakub. > > > >> > > > >>I have the IPA plugin already written (not tested yet), so I believe > > > >>this is the final version if there won't be any review comments. > > > >//lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > > > > >OK. One more time :-) > > > >I have slightly enhanced debug messages. > > > > > > I forgot to check if a server is already present in the list. The fifth > > > patch was split in two. > > > > > > > > > > Patch 0001: resolv: add resolv_get_domain request to resolv utils > > Mostly OK, can you just explain this part of the code? > > > > 152 dns_domain = talloc_strdup(state, dns_domain); > > 153 if (dns_domain == NULL) { > > 154 return ENOMEM; > > 155 } > > 156 > > 157 *_dns_domain = talloc_steal(mem_ctx, dns_domain); > > 158 > > 159 return EOK; > > > > Isn't it better to just: > > *_dns_domain = talloc_strdup(mem_ctx, dns_domain); > > ? > > > > Patch 0002: resolv: add resolv_discover_srv request to resolv utils > > Is it necessary to strdup the input variables? It might be safer in > > case this request outlives the request that created it but in most cases > > it's not really needed. The discovery_domain, on the other hand is > > neither checked not duped. > > > > Patch 0003: DNS sites support - SRV lookup plugin interface > > Code-wise looks good, but I have two questions related to the behaviour. > > > > Why can't the setter overwrite the plugin interface when it's already > > set? Isn't is nice for cases like LDAP where LDAP might set its generic > > plugin first and then IPA which uses LDAP underneath would reset them? > > > > Patch 0004: DNS sites support - SRV DNS lookup plugin > > Please do not use constructs like this unless EOK is expected: > > > > 66 immediately: > > 67 if (ret == EOK) { > > 68 tevent_req_done(req); > > 69 } else { > > > > At the very least it's confusing Coverity and showing up as "dead code". > > > > Same question about strdup-ing input parameters as for patch #2. > > > > I'm not really fond of the name "be_fo_set_standard_srv_lookup_plugin" > > -- standard by what means? What about using the same naming standard as > > in fail_over_srv.c and using be_fo_set_dns_srv_lookup_plugin ? > > > > Also since be_fo_set_standard_srv_lookup_plugin can fail and then the > > failover would not really be usable, it should return errno and users > > should check it. > > > > Patch 0005: fail over - add function to insert multiple servers to the > > list > > Ack > > > > Patch 0006: DNS sites support - replace SRV lookup code with a plugin > > call > > Did you check if the code can be compiled with this patch? If it can, > > then it's fine that the lookups don't work but I would prefer the > > patches to compile on their own so that git-bisect is usable. > > > > Otherwise ack. > > > > Patch 0007: DNS sites support - use SRV DNS lookup plugin in all providers > > Mostly ack, but when I looked at how the host name is selected in LDAP > > initialization, it seemed to be that this could be moved down the stack > > either to be_fo_set_standard_srv_lookup_plugin or even lower to > > resolv_utils. Anyway, looks ugly in the generic LDAP code :) > > > > Patch 0008: DNS sites support - make fo_discover_srv request visible > > I don't get the point of this patch, why not make the functions pubilc > > from the start? > > > > So far I haven't tested the patches, just read the code to give some > > feedback. I'll reply again when I've tested it. > > My testing consisted of positive tests with the IPA (including the one > patch in the other thread) and LDAP backends, those went fine. Negative > tests worked OK, too but revealed one probably uninitialized variable: > > [sssd[be[ipaldap]]] [fo_set_port_status] (0x0100): Marking port 0 of server > '(no name)' as 'not working' > [sssd[be[ipaldap]]] [resolve_srv_done] (0x0040): Unable to resolve SRV > [1432158224]: SRV record not found > ^^^^^^ > seems wrong > [sssd[be[ipaldap]]] [set_srv_data_status] (0x0100): Marking SRV lookup of > service 'LDAP' as 'not resolved' > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x0080): Couldn't resolve > server (SRV lookup meta-server), resolver returned (1432158224) > [sssd[be[ipaldap]]] [be_resolve_server_process] (0x1000): Trying with the > next one! > > This occured when I queried a server that didn't have the query stored > at all. > > The query refresh seemed to worked OK also (I tested by manually > modifiying the timeout). Except for the uninitialized variable, I think > the patches are working good and we just need to resolve the minor code > concerns above.
What exactly looks wrong to you? The high error code? That's OK - it is custom sss error. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
