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

[Jan Cholasta]

On 15.1.2014 13:55, Alexander Bokovoy wrote:
> 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.

Well, now it's multiple fixes in a single patch. What I meant to end up 
with is single fix per single patch (see attachment).

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-179-Do-not-start-the-service-in-stopped_service-if-it-wa.patch
Type: text/x-patch
Size: 1115 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140115/75ac9ac9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-abbra-0131-ipaserver-install-installutils-clean-up-properly-aft-1.patch
Type: text/x-patch
Size: 1900 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140115/75ac9ac9/attachment-0001.bin>