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
>From b1d43e12aeecf5d5a98aad3f12c3d6bb8dd46453 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 15 Oct 2013 17:49:07 +0000
Subject: [PATCH 08/25] Do not start the service in stopped_service if it was
not running before.
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)
--
1.8.3.1
>From 0a05510755b1929867a72561de0b939adbd159ed Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@redhat.com>
Date: Wed, 15 Jan 2014 17:26:10 +0100
Subject: [PATCH] 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.
---
ipaserver/install/installutils.py | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index cc53f75..c6376d8 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
@@ -827,6 +828,8 @@ def stopped_service(service, instance_name=""):
# 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.5.2
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
