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.