[PATCH 1/2] Close file descriptors when running ntpdate (#1083978)
Vratislav Podzimek
vpodzime at redhat.com
Thu Dec 4 15:31:10 UTC 2014
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 at 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
More information about the anaconda-patches
mailing list