[SSSD] [PATCH] DNS site support 1st wave - generic SRV lookup plugin

Jakub Hrozek jhrozek at redhat.com
Wed Apr 10 11:28:12 UTC 2013


On Tue, Apr 09, 2013 at 02:35:24PM +0200, Pavel Březina wrote:
> >>>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.
> 

You still need to check the result of strdup for NULL :)

Otherwise Ack.

> >>>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.
> 

I was actually thinking about it a bit and I think that there should be
no guesswork in an API that is provided for other modules. The API
should be able to survive its parent request going away.

No need to revert this part now, I will file a ticket and we can do this
minor change post-beta.

Sorry I changed my mind after asking you to change the code.

Ack for now.

> >>>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.

You're right, correct.

> 
> >>>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.

There is still one part like this in patch #1, can you remove it too
when you fix the strdup?

> 
> >>>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

Still 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.
> 

Ack

> >>>
> >>>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().
> 

Ack

> >>>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.

I couldn't compile these patches without the IPA discovery patch on top,
I was getting:
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c: In
function 'sssm_ipa_id_init':
/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_init.c:213:5:
error: implicit declaration of function
'be_fo_set_standard_srv_lookup_plugin'
[-Werror=implicit-function-declaration]

Probably some hunk got squashed into the IPA discovery patch?

Simple testing with all these patches and the IPA one on top went fine.



More information about the sssd-devel mailing list