On Thu, 2014-12-04 at 10:09 -0500, David Shea wrote:
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.
That might be nice to change/fix in any case, but
I think our
"execWithRedirect machinery" is something we don't really need for
running 'ntpdate -q' on a server when we are only interested in the
return code. It would just spam our logs and cause some delays due to
the lock.
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic