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

[Jan Cholasta]

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

-- 
Jan Cholasta