[Freeipa-devel] [PATCH] 232 Increase service startup timeout default

[Alexander Bokovoy]

On Wed, 15 Jan 2014, Jan Cholasta wrote:
>On 15.1.2014 13:21, Alexander Bokovoy wrote:
>>On Wed, 15 Jan 2014, Jan Cholasta wrote:
>>>On 15.1.2014 12:17, Martin Kosek wrote:
>>>>On 01/15/2014 11:20 AM, Alexander Bokovoy wrote:
>>>>>On Wed, 15 Jan 2014, Jan Cholasta wrote:
>>>>>>Hi,
>>>>>>
>>>>>>the attached patch should fix
>>>>>><https://fedorahosted.org/freeipa/ticket/4078>.
>>>>>>
>>>>>>I have also attached patch 179, which fixes a related bug in
>>>>>>certificate
>>>>>>renewal.
>>>>>
>>>>>NACK for this part:
>>>>>>This fixes a possible NSS database corruption in renew_ca_cert.
>>>>>>---
>>>>>>ipaserver/install/installutils.py | 3 ---
>>>>>>1 file changed, 3 deletions(-)
>>>>>>
>>>>>>diff --git a/ipaserver/install/installutils.py
>>>>>>b/ipaserver/install/installutils.py
>>>>>>index 67eabc2..0ba9c2e 100644
>>>>>>--- a/ipaserver/install/installutils.py
>>>>>>+++ b/ipaserver/install/installutils.py
>>>>>>@@ -820,9 +820,6 @@ def stopped_service(service, instance_name=""):
>>>>>>        root_logger.debug('Service %s%s is not running, continue.',
>>>>>>service,
>>>>>>                          log_instance_name)
>>>>>>        yield
>>>>>>-        root_logger.debug('Starting %s%s.', service,
>>>>>>log_instance_name)
>>>>>>-        ipaservices.knownservices[service].start(instance_name)
>>>>>>-        return
>>>>>>    else:
>>>>>>        # Stop the service, do the required stuff and start it again
>>>>>>        root_logger.debug('Stopping %s%s.', service,
>>>>>>log_instance_name)
>>>>>You need to wrap yield into try: finally: block. I have a patch for
>>>>>similar case in private_cache() few lines above this code.
>>>
>>>There is no cleanup needed for this particular yield. Also this is not
>>>really the concern of the patch, it does exactly what the commit
>>>message says. Since you are already fixing a similar case, I would
>>>suggest you amend your patch with any changes you deem necessary, I
>>>don't see why a single fix should be dispersed among multiple patches.
>>Patch attached, it obsoletes your patch 179.
>
>Thanks, but I don't understand why you squashed my patch 179 into 
>your patch, the fixes are for separate issues (yield exception 
>handling vs. previously stopped service being started).
Because you just said above:
>>>suggest you amend your patch with any changes you deem necessary, I
>>>don't see why a single fix should be dispersed among multiple patches.
"a single fix" is now not dispersed among multiple patches.

-- 
/ Alexander Bokovoy