Re: [Freeipa-devel] [PATCH] 71 Propagate SIGINT to child process in ipautil.run

2012-03-23 Thread Martin Kosek
On Fri, 2012-03-23 at 10:43 +0100, Jan Cholasta wrote:
 On 22.3.2012 16:35, Martin Kosek wrote:
  On Tue, 2012-03-20 at 17:48 +0100, Jan Cholasta wrote:
  Propagate SIGINT to child process in ipautil.run.
 
  Wait for the child process to terminate before continuing.
 
  Do cleanup on KeyboardInterrupt rather than in custom SIGINT handler in
  ipa-replica-conncheck.
 
  https://fedorahosted.org/freeipa/ticket/2127
 
  Honza
 
  This looks and works OK, I have just one minor issue. Isn't the extra
  p.wait() you added to the standard run() path redundant? p.communicate()
  should do the job of waiting until the child process terminates.
 
  +try:
  +p = subprocess.Popen(args, stdin=p_in, stdout=p_out,
  stderr=p_err,
  + close_fds=True, env=env)
  +stdout,stderr = p.communicate(stdin)
  +stdout,stderr = str(stdout), str(stderr)# Make pylint happy
  +p.wait()
  +except KeyboardInterrupt:
 
 
 You are of course right, I guess I should read documentation more carefully.
 
  Martin
 
 
 Also, SIGINT is already propagated to the child process, we just need to 
 wait for it to terminate.

Yeah, with p.wait() there is also no left-over in a form of zombie
process.

 
 Updated patch attached.
 
 Honza
 

ACK. Pushed to master, ipa-2-2.

Martin

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


Re: [Freeipa-devel] [PATCH] 71 Propagate SIGINT to child process in ipautil.run

2012-03-22 Thread Martin Kosek
On Tue, 2012-03-20 at 17:48 +0100, Jan Cholasta wrote:
 Propagate SIGINT to child process in ipautil.run.
 
 Wait for the child process to terminate before continuing.
 
 Do cleanup on KeyboardInterrupt rather than in custom SIGINT handler in 
 ipa-replica-conncheck.
 
 https://fedorahosted.org/freeipa/ticket/2127
 
 Honza

This looks and works OK, I have just one minor issue. Isn't the extra
p.wait() you added to the standard run() path redundant? p.communicate()
should do the job of waiting until the child process terminates.

+try:
+p = subprocess.Popen(args, stdin=p_in, stdout=p_out,
stderr=p_err,
+ close_fds=True, env=env)
+stdout,stderr = p.communicate(stdin)
+stdout,stderr = str(stdout), str(stderr)# Make pylint happy
+p.wait()
+except KeyboardInterrupt:

Martin

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


[Freeipa-devel] [PATCH] 71 Propagate SIGINT to child process in ipautil.run

2012-03-20 Thread Jan Cholasta

Propagate SIGINT to child process in ipautil.run.

Wait for the child process to terminate before continuing.

Do cleanup on KeyboardInterrupt rather than in custom SIGINT handler in 
ipa-replica-conncheck.


https://fedorahosted.org/freeipa/ticket/2127

Honza

--
Jan Cholasta
From a4941a69f28858a2b3492831ccc79b7e1bfaad83 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 20 Mar 2012 12:29:36 -0400
Subject: [PATCH] Propagate SIGINT to child process in ipautil.run.

Wait for the child process to terminate before continuing.

Do cleanup on KeyboardInterrupt rather than in custom SIGINT handler in
ipa-replica-conncheck.

https://fedorahosted.org/freeipa/ticket/2127
---
 install/tools/ipa-replica-conncheck |   13 +
 ipapython/ipautil.py|   22 ++
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index 44b3caa..23411a3 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -158,12 +158,10 @@ def clean_responders(responders):
 responders.remove(responder)
 
 def sigterm_handler(signum, frame):
-print_info(\nCleaning up...)
-
-global RESPONDERS
-clean_responders(RESPONDERS)
-
-sys.exit(1)
+# do what SIGINT does (raise a KeyboardInterrupt)
+sigint_handler = signal.getsignal(signal.SIGINT)
+if callable(sigint_handler):
+sigint_handler(signum, frame)
 
 def configure_krb5_conf(realm, kdc, filename):
 
@@ -268,7 +266,6 @@ def main():
 root_logger.debug(missing options might be asked for interactively later\n)
 
 signal.signal(signal.SIGTERM, sigterm_handler)
-signal.signal(signal.SIGINT, sigterm_handler)
 
 required_ports = BASE_PORTS
 if options.check_ca:
@@ -384,6 +381,7 @@ if __name__ == __main__:
 except SystemExit, e:
 sys.exit(e)
 except KeyboardInterrupt:
+print_info(\nCleaning up...)
 sys.exit(1)
 except RuntimeError, e:
 sys.exit(e)
@@ -395,4 +393,3 @@ if __name__ == __main__:
 os.remove(file_name)
 except OSError:
 pass
-
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 416ebf5..0c646c4 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -26,7 +26,6 @@ IPA_BASEDN_INFO = 'ipa v2.0'
 
 import string
 import tempfile
-from ipapython.ipa_log_manager import *
 import subprocess
 import random
 import os, sys, traceback, readline
@@ -37,14 +36,15 @@ import urllib2
 import socket
 import ldap
 import struct
-
-from ipapython import ipavalidate
 from types import *
-
 import re
 import xmlrpclib
 import datetime
 import netaddr
+import signal
+
+from ipapython.ipa_log_manager import *
+from ipapython import ipavalidate
 from ipapython import config
 try:
 from subprocess import CalledProcessError
@@ -259,10 +259,16 @@ def run(args, stdin=None, raiseonerr=True,
 p_out = subprocess.PIPE
 p_err = subprocess.PIPE
 
-p = subprocess.Popen(args, stdin=p_in, stdout=p_out, stderr=p_err,
- close_fds=True, env=env)
-stdout,stderr = p.communicate(stdin)
-stdout,stderr = str(stdout), str(stderr)# Make pylint happy
+try:
+p = subprocess.Popen(args, stdin=p_in, stdout=p_out, stderr=p_err,
+ close_fds=True, env=env)
+stdout,stderr = p.communicate(stdin)
+stdout,stderr = str(stdout), str(stderr)# Make pylint happy
+p.wait()
+except KeyboardInterrupt:
+os.kill(p.pid, signal.SIGINT)
+p.wait()
+raise
 
 # The command and its output may include passwords that we don't want
 # to log. Run through the nolog items.
-- 
1.7.7.6

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