On Wed, Apr 10, 2013 at 02:22:26PM +0200, Pavel Březina wrote:
> On 04/10/2013 01:28 PM, Jakub Hrozek wrote:
> >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 :)
>
> Right, thanks.
Ack
>
> >
> >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.
> >
https://fedorahosted.org/sssd/ticket/1875
> >Sorry I changed my mind after asking you to change the code.
> >
> >Ack for now.
Ack
> >
> >>>>>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
>
> Done.
>
> >when you fix the strdup?
Ack
> >
> >>
> >>>>>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?
>
> You are correct. Fixed.
>
Ack
> >
> >Simple testing with all these patches and the IPA one on top went fine.
Simple SRV testing also passed with this round of patches.
All patches are acked now.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
