[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