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.

--
/ Alexander Bokovoy
>From a201bd425d988257769f096093198e16c23e7065 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@redhat.com>
Date: Wed, 15 Jan 2014 14:16:49 +0200
Subject: [PATCH 2/2] ipaserver/install/installutils: clean up properly after
 yield

When a context to which we yield generates exception, the code in
private_ccache() and stopped_service() didn't get called for cleanup.

Additionally, in stopped_service() one should not start a previously
stopped service after the context returned control to us because the
original state was with stopped service already.

Fixes https://fedorahosted.org/freeipa/ticket/4078
---
 ipaserver/install/installutils.py | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/ipaserver/install/installutils.py 
b/ipaserver/install/installutils.py
index cc53f75..67216f1 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -786,15 +786,16 @@ def private_ccache(path=None):
 
     os.environ['KRB5CCNAME'] = path
 
-    yield
-
-    if original_value is not None:
-        os.environ['KRB5CCNAME'] = original_value
-    else:
-        os.environ.pop('KRB5CCNAME')
+    try:
+        yield
+    finally:
+        if original_value is not None:
+            os.environ['KRB5CCNAME'] = original_value
+        else:
+            os.environ.pop('KRB5CCNAME')
 
-    if os.path.exists(path):
-        os.remove(path)
+        if os.path.exists(path):
+            os.remove(path)
 
 
 @contextmanager
@@ -820,13 +821,13 @@ 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)
         ipaservices.knownservices[service].stop(instance_name)
-        yield
-        root_logger.debug('Starting %s%s.', service, log_instance_name)
-        ipaservices.knownservices[service].start(instance_name)
+        try:
+            yield
+        finally:
+            root_logger.debug('Starting %s%s.', service, log_instance_name)
+            ipaservices.knownservices[service].start(instance_name)
-- 
1.8.4.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to