On 04/08/2013 10:55 PM, Jakub Hrozek wrote:
On Mon, Apr 08, 2013 at 10:38:47PM +0200, Jakub Hrozek wrote:
> 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.
Segfault fixed.
>> 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);
>> ?
I think it is better to not touch output parameters at all in case of
failure. But I don't feel so strong about it. Done.
>> 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.
I remember fixing similar issue in resolver some time ago. So that
probably made me write more robust code here.
58b335985e75672e4de699351ab1182cbd7aa990
But you are right that it is unlikely to happen here.
Removed strdup() and added check for discovery_domain.
>> 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?
We agreed that id_provider is authoritative for plugin choise. And
id_provider is initialized at first. Therefore it is not desirable to
override it. It is described in #0007 commit message, but maybe I should
put a comment here as well.
>> 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".
Done.
>> Same question about strdup-ing input parameters as for patch
#2.
Done.
>>
>> 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 ?
Done.
>> 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.
Done.
>> 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.
Yes, it compiles fine.
>>
>> 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 :)
Moved to be_fo_set_dns_srv_lookup_plugin().
>> 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?
Darn, this belonged to the IPA plugin patch. But yes, it can be also
squashed in the forth patch.
>> 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.
You also need to put the new header files (src/providers/fail_over_srv.h
and src/providers/ipa/ipa_srv.h I think) into the nodist rule in
Makefile.am
Done.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel