[Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI

[thierry bordaz]

On 01/21/2016 05:38 PM, Martin Babinsky wrote:
> On 01/21/2016 05:22 PM, Rob Crittenden wrote:
>> Martin Babinsky wrote:
>>> On 01/21/2016 01:37 PM, thierry bordaz wrote:
>>
>>> 6.)
>>>
>>> +        while attempt != MAX_WAIT:
>>> +            try:
>>> +                entries = conn.get_entries(sharedcfgdn,
>>> scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
>>> +                break
>>> +            except errors.NotFound:
>>> +                root_logger.debug("So far enable not find DNA shared
>>> config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
>>> sharedcfgdn))
>>> +                attempt = attempt + 1
>>> +                time.sleep(2)
>>> +                continue
>>> +
>>> +        # safety checking
>>> +        # there is no return, if there are several entries, as a
>>> workaround of #5510
>>> +        if len(entries) != 1:
>>>
>>> I am quite afraid what would happen if the server does not return any
>>> entries until 30 s timeout. The code will then continue to the 
>>> condition
>>> which can potentially test an uninitialized variable and blow up with
>>> 'NameError'. This should be handled more robustly, e. g. raise an
>>> exception when a timeout is reached and no entries were returned.
>>
>> I agree, but note that it is a 60s timeout (30 tries x 2 second sleeps).
>>
> Right, looks like I forgot how to math.
>
> I was thinking that this loop could be rewritten in a safer way like 
> this:
> """
>         for i in range(0, max_wait + 1):
>             try:
>                 entries = conn.get_entries(sharedcfgdn, 
> scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
>                 break
>             except errors.NotFound:
>                 root_logger.debug("So far enable not find DNA shared 
> config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn, 
> sharedcfgdn))
>                 time.sleep(2)
>         else:
>             raise RuntimeError("Could not get dnaHostname entries in 
> {} seconds".format(max_wait * 2))
> """
>
> the else: branch will be executed only after the iterator (range in 
> this case because Py3 compatibility) is exhausted thus handling the 
> timeout.

Hello Martin,

Thanks for this very nice code !
I was a bit afraid of aborting a full install if it was not able to 
update the localhost shared config, this is why it just logged a 
message. Finally, raising an exception is the thing to do.

>
>> This will blow up if something other than NotFound is returned (e.g.
>> connection error), and maybe that's ok.
>>
> Other exceptions do not worry me, only repeated NotFound due to some 
> bad magic preventing the entries to appear which can lead to unhandled 
> timeout.
>
>> The continue is not needed.
>>
>> rob
>>
>
>