On 12/04/2014 09:23 AM, Vratislav Podzimek wrote:
Using os.system() means that the child process inherits all file
descriptors
from the parent. In case we have some file opened in the time when some
long-running process is forked, there is a high chance that the opened file
descriptor can cause issues. Like in the referenced bug where it prevented
filesystem from being unmounted.
Using subprocess.Popen() with close_fds=True is the easiest way to make sure
file descriptors are closed in the child process. All other ways seem to be
crazy machinery.
Signed-off-by: Vratislav Podzimek <vpodzime(a)redhat.com>
---
pyanaconda/ntp.py | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/pyanaconda/ntp.py b/pyanaconda/ntp.py
index 0014a9d..a54e807 100644
--- a/pyanaconda/ntp.py
+++ b/pyanaconda/ntp.py
@@ -27,6 +27,7 @@ import re
import os
import tempfile
import shutil
+import subprocess
from pyanaconda import iutil
from pyanaconda.threads import threadMgr, AnacondaThread
@@ -50,11 +51,13 @@ def ntp_server_working(server):
"""
- #FIXME: It would be nice to use execWithRedirect here, but it is not
- # thread-safe and hangs if this function is called from threads.
- # By using tee (and block-buffered pipes) it is also much slower.
- #we just need to know the exit status
- retc = os.system("ntpdate -q %s &>/dev/null" % server)
+ # using iutil.execWithRedirect here would mean that if this function was run
+ # multiple times from multiple threads in the same time, the execution would
+ # be serialized because of the program_log lock
+ proc = subprocess.Popen(["ntpdate", "-q", server],
close_fds=True)
+
+ # we just need to know the exit status
+ retc = proc.wait()
return retc == 0
If you're willing to mess with _run_program some more, you could use
execWithRedirect by changing where the program log lock is held. On
master proc.communicate() happens outside of the lock, and the lock is
only held for the log.info calls.