The overall goal is to ensure that any time we start a new process, it should be started with a clean set of signal handlers (in particular, make sure that SIGPIPE is set to SIG_DFL instead of the SIG_IGN set by python) and no file descriptors from the parent make it through exec() other than stdin, stdout and stderr.
The first five are cleanups in the main anaconda script that I hope will be uncontroversial, one of them obviously a typo that I pushed a couple weeks ago. Oops.
Patches 6, 7, and 15 are related to the SIGCHLD handler. We don't want to throw an exception if X or metacity exits before anaconda during a reboot, so clear the watched process list in exitHandler. Patch 6 rearranges the anaconda script in order to catch exceptions from watched processes, so we can do the ipmi stuff when X crashes. Patch 15 moves the watch methods into iutil and modifies them to use Popen objects instead of a custom fork-exec protocol.
The "Simplify iutil.execReadlines" patch we've discussed before, I've just split the bits not directly related to execReadlines into separate commits and expanded the commit message to hopefully explain and address the concerns from last time. The rest of the patches are for creating a process starting interface flexible enough for all of our process starts, and getting everyone to use it.
I'm not real happy about patch 7, but I think we ought to do something in order to send the ipmi report. Some discussion points:
- I went with raise instead of sys.exit because I thought it would be unhelpful if anaconda rebooted in such a case. Is there something better we could do? - Should we catch Exception, instead? I don't think we do any ipmi stuff if something crashes and we get a meh handler, but I'm not 100% sure about that. ExitErrors will always be delivered to the main thread, but if we catch Exception, would we need a handler in the threading module as well? - None of this ever actually gets called, because gdk calls _exit(1) (!!) when it loses the X connection. How do we work around that? * If we override the error handlers with our own XSetErrorHandler and XSetIOErrorHandler calls, there will still be a gap between Gdk init and our calls where X could crash and cause anaconda to exit, also having XLib calls in anaconda would be super gross * If we launch the graphical stuff in a separate process that would be medium gross * If we ask the Gtk to not freaking exit in a library call they probably won't - Also how much of this stuff needs to go to rhel7-branch
David Shea (17): Fix a typo in a comment Import gettext in iutil instead of passing the module reference to iutil Import _ from the i18n module instead of hand-crafting a copy of it Remove uses of the deprecated string.split method. Remove the exitCode parameter from exitHandler. Clear the list of watched PIDs before exiting. Handle watched processes exiting. Lock program_log_lock closer to where the log is written. Add a method startProgram to handle process starting Add an option to startProgram to reset signal handlers. Add close_fds to the Popen call. Simplify iutil.execReadlines. Add an option to only capture stdout with execWithCapture Close file descriptors while daemonizing auditd Move process watching to iutil. Move the X startup logic to iutil Always use iutil to start processes.
anaconda | 384 ++++++++++---------------------- pyanaconda/anaconda_log.py | 4 +- pyanaconda/ihelp.py | 4 +- pyanaconda/isys/auditd.c | 5 + pyanaconda/iutil.py | 373 ++++++++++++++++++++++++------- pyanaconda/kickstart.py | 32 +-- pyanaconda/rescue.py | 4 +- pyanaconda/ui/gui/spokes/network.py | 6 +- pyanaconda/ui/tui/spokes/askvnc.py | 5 +- pyanaconda/ui/tui/spokes/shell_spoke.py | 6 +- pyanaconda/vnc.py | 14 +- scripts/instperf | 3 +- tests/pyanaconda_tests/iutil_test.py | 227 +++++++++++++++---- 13 files changed, 615 insertions(+), 452 deletions(-)
--- anaconda | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/anaconda b/anaconda index cc8aa43..0e25eda 100755 --- a/anaconda +++ b/anaconda @@ -1005,7 +1005,7 @@ if __name__ == "__main__": signal.signal(signal.SIGTERM, lambda num, frame: sys.exit(1))
# synchronously-delivered signals such as SIGSEGV and SIGILL cannot be - # handled property from python, so install signal handlers from the C + # handled properly from python, so install signal handlers from the C # function in isys. isys.installSyncSignalHandlers()
I mean, the module is always going to be gettext, so doing it this way just seems kind of silly now. --- anaconda | 2 +- pyanaconda/iutil.py | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/anaconda b/anaconda index 0e25eda..64cbdc8 100755 --- a/anaconda +++ b/anaconda @@ -998,7 +998,7 @@ if __name__ == "__main__":
from pyanaconda.anaconda import Anaconda anaconda = Anaconda() - iutil.setup_translations(gettext) + iutil.setup_translations()
# reset python's default SIGINT handler signal.signal(signal.SIGINT, signal.SIG_IGN) diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 0a70917..a849df8 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -34,6 +34,7 @@ import re from threading import Thread from Queue import Queue, Empty from urllib import quote, unquote +import gettext
from pyanaconda.flags import flags from pyanaconda.constants import DRACUT_SHUTDOWN_EJECT, TRANSLATIONS_UPDATE_DIR, UNSUPPORTED_HW @@ -397,7 +398,7 @@ def parseNfsUrl(nfsurl):
return (options, host, path)
-def add_po_path(module, directory): +def add_po_path(directory): """ Looks to see what translations are under a given path and tells the gettext module to use that path as the base dir """ for d in os.listdir(directory): @@ -409,12 +410,12 @@ def add_po_path(module, directory): if not basename.endswith(".mo"): continue log.info("setting %s as translation source for %s", directory, basename[:-3]) - module.bindtextdomain(basename[:-3], directory) + gettext.bindtextdomain(basename[:-3], directory)
-def setup_translations(module): +def setup_translations(): if os.path.isdir(TRANSLATIONS_UPDATE_DIR): - add_po_path(module, TRANSLATIONS_UPDATE_DIR) - module.textdomain("anaconda") + add_po_path(TRANSLATIONS_UPDATE_DIR) + gettext.textdomain("anaconda")
def _run_systemctl(command, service): """
--- anaconda | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/anaconda b/anaconda index 64cbdc8..f16bce6 100755 --- a/anaconda +++ b/anaconda @@ -913,8 +913,7 @@ if __name__ == "__main__": initThreading() from pyanaconda.threads import threadMgr
- import gettext - _ = lambda x: gettext.ldgettext("anaconda", x) + from pyanaconda.i18n import _
from pyanaconda import constants from pyanaconda.addons import collect_addon_paths
Strings can do that themselves nowadays. This gets us more ready for python3 and gets an import out of the anaconda script. --- anaconda | 4 +--- scripts/instperf | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/anaconda b/anaconda index f16bce6..19441b4 100755 --- a/anaconda +++ b/anaconda @@ -690,7 +690,7 @@ def setupDisplay(anaconda, options, addons=None):
# Only consider vncconnect when vnc is a param if options.vncconnect: - cargs = string.split(options.vncconnect, ":") + cargs = options.vncconnect.split(":") vncS.vncconnecthost = cargs[0] if len(cargs) > 1 and len(cargs[1]) > 0: if len(cargs[1]) > 0: @@ -964,8 +964,6 @@ if __name__ == "__main__":
from pyanaconda import isys
- import string - from pyanaconda import iutil
iutil.ipmi_report(constants.IPMI_STARTED) diff --git a/scripts/instperf b/scripts/instperf index f0d53b2..daa91a7 100755 --- a/scripts/instperf +++ b/scripts/instperf @@ -1,7 +1,6 @@ #!/usr/bin/python
import logging -from string import split from subprocess import Popen, PIPE import time
@@ -12,7 +11,7 @@ import time def topProcesses(): output = Popen(["ps", "-eo", "comm,rss", "--sort", "-rss", "--no-headers"], stdout=PIPE).communicate()[0] top5 = output.split("\n")[:5] - return ",".join(map(lambda (a,b): a+":"+b, map(split, top5))) + return ",".join("%s:%s" % (proc[0], proc[1]) for proc in (s.split() for s in top5))
def logit(): buffers = 0
On 09/22/2014 10:52 AM, Chris Lumens wrote:
Strings can do that themselves nowadays. This gets us more ready for python3 and gets an import out of the anaconda script.
Any idea why pylint hasn't been complaining about these all along?
We used to ignore the warnings with pylint-false-positives, and by the time it was removed from false-positives the check was also removed from pylint, since the check was more heavy-handed than it should have been. https://bitbucket.org/logilab/pylint/issue/3
We used to ignore the warnings with pylint-false-positives, and by the time it was removed from false-positives the check was also removed from pylint, since the check was more heavy-handed than it should have been. https://bitbucket.org/logilab/pylint/issue/3
Ah, thanks. Well we import it only sparingly so perhaps we should add it back into the list of deprecated modules and pragma our way out of it when we know we're using something useful from it.
- Chris
On 09/22/2014 11:12 AM, Chris Lumens wrote:
We used to ignore the warnings with pylint-false-positives, and by the time it was removed from false-positives the check was also removed from pylint, since the check was more heavy-handed than it should have been. https://bitbucket.org/logilab/pylint/issue/3
Ah, thanks. Well we import it only sparingly so perhaps we should add it back into the list of deprecated modules and pragma our way out of it when we know we're using something useful from it.
Seems like an ok idea. While going through the string uses I also noticed that we use .letters in several places, which does not exist in Python 3 and we meant .ascii_letters anyway.
The string module is about half-deprecated, so we need to be careful about which parts of it we use. We don't use it too much, so warn about any use and filter out the correct ones with pylint pragmas.
Remove the uses of string.split, since strings can do that themselves nowadays and string.split is removed in python3, and string.letters, since this is also removed in python3 and we actually meant string.ascii_letters. --- anaconda | 4 +--- pyanaconda/bootloader.py | 5 +++-- pyanaconda/constants.py | 3 ++- pyanaconda/iutil.py | 3 ++- pyanaconda/ui/gui/spokes/network.py | 5 +++-- pyanaconda/users.py | 5 +++-- scripts/instperf | 3 +-- tests/pylint/pylint-one.sh | 1 + tests/pylint/runpylint.sh | 4 ++++ 9 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/anaconda b/anaconda index f16bce6..19441b4 100755 --- a/anaconda +++ b/anaconda @@ -690,7 +690,7 @@ def setupDisplay(anaconda, options, addons=None):
# Only consider vncconnect when vnc is a param if options.vncconnect: - cargs = string.split(options.vncconnect, ":") + cargs = options.vncconnect.split(":") vncS.vncconnecthost = cargs[0] if len(cargs) > 1 and len(cargs[1]) > 0: if len(cargs[1]) > 0: @@ -964,8 +964,6 @@ if __name__ == "__main__":
from pyanaconda import isys
- import string - from pyanaconda import iutil
iutil.ipmi_report(constants.IPMI_STARTED) diff --git a/pyanaconda/bootloader.py b/pyanaconda/bootloader.py index c31c0bf..9370c0a 100644 --- a/pyanaconda/bootloader.py +++ b/pyanaconda/bootloader.py @@ -1099,12 +1099,13 @@ class GRUB(BootLoader): if not self.password: raise BootLoaderError("cannot encrypt empty password")
- import string + # Used for ascii_letters and digits constants + import string # pylint: disable=deprecated-module import crypt import random salt = "$6$" salt_len = 16 - salt_chars = string.letters + string.digits + './' + salt_chars = string.ascii_letters + string.digits + './'
rand_gen = random.SystemRandom() salt += "".join(rand_gen.choice(salt_chars) for i in range(salt_len)) diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py index e70ae1e..e76b7ee 100644 --- a/pyanaconda/constants.py +++ b/pyanaconda/constants.py @@ -19,7 +19,8 @@ # Author(s): Erik Troan ewt@redhat.com #
-import string +# Used for digits, ascii_letters, punctuation constants +import string # pylint: disable=deprecated-module from pyanaconda.i18n import N_
# Use -1 to indicate that the selinux configuration is unset diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index a849df8..a997366 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -27,7 +27,8 @@ import os.path import errno import subprocess import unicodedata -import string +# Used for ascii_lowercase, ascii_uppercase constants +import string # pylint: disable=deprecated-module import tempfile import types import re diff --git a/pyanaconda/ui/gui/spokes/network.py b/pyanaconda/ui/gui/spokes/network.py index 50b613e..91b7276 100644 --- a/pyanaconda/ui/gui/spokes/network.py +++ b/pyanaconda/ui/gui/spokes/network.py @@ -49,7 +49,8 @@ from gi.repository import GLib, GObject, Pango, Gio, NetworkManager, NMClient import dbus import dbus.service import subprocess -import string +# Used for ascii_letters and hexdigits constants +import string # pylint: disable=deprecated-module from uuid import uuid4
from dbus.mainloop.glib import DBusGMainLoop @@ -1325,7 +1326,7 @@ class SecretAgent(dbus.service.Object): if len(value) in (10, 26): return all(c in string.hexdigits for c in value) elif len(value) in (5, 13): - return all(c in string.letters for c in value) + return all(c in string.ascii_letters for c in value) else: return False elif secret['wep_key_type'] == NetworkManager.WepKeyType.PASSPHRASE: diff --git a/pyanaconda/users.py b/pyanaconda/users.py index 82cd2d3..b5b076b 100644 --- a/pyanaconda/users.py +++ b/pyanaconda/users.py @@ -20,7 +20,8 @@ #
import libuser -import string +# Used for ascii_letters and digits constants +import string # pylint: disable=deprecated-module import crypt import random import tempfile @@ -115,7 +116,7 @@ def cryptPassword(password, algo=None): saltstr = salts[algo]
for _i in range(saltlen): - saltstr = saltstr + random.choice (string.letters + + saltstr = saltstr + random.choice (string.ascii_letters + string.digits + './')
cryptpw = crypt.crypt (password, saltstr) diff --git a/scripts/instperf b/scripts/instperf index f0d53b2..daa91a7 100755 --- a/scripts/instperf +++ b/scripts/instperf @@ -1,7 +1,6 @@ #!/usr/bin/python
import logging -from string import split from subprocess import Popen, PIPE import time
@@ -12,7 +11,7 @@ import time def topProcesses(): output = Popen(["ps", "-eo", "comm,rss", "--sort", "-rss", "--no-headers"], stdout=PIPE).communicate()[0] top5 = output.split("\n")[:5] - return ",".join(map(lambda (a,b): a+":"+b, map(split, top5))) + return ",".join("%s:%s" % (proc[0], proc[1]) for proc in (s.split() for s in top5))
def logit(): buffers = 0 diff --git a/tests/pylint/pylint-one.sh b/tests/pylint/pylint-one.sh index 8cf4248..5f6ac01 100755 --- a/tests/pylint/pylint-one.sh +++ b/tests/pylint/pylint-one.sh @@ -24,6 +24,7 @@ pylint_output="$(pylint \ gi.overrides.__path__[0:0] = (os.environ["ANACONDA_WIDGETS_OVERRIDES"].split(":") if "ANACONDA_WIDGETS_OVERRIDES" in os.environ else [])' \ $DISABLED_WARN_OPTIONS \ $DISABLED_ERR_OPTIONS \ + $EXTRA_OPTIONS \ $NON_STRICT_OPTIONS "$@" 2>&1 | \ egrep -v -f "$FALSE_POSITIVES" \ )" diff --git a/tests/pylint/runpylint.sh b/tests/pylint/runpylint.sh index a76b4c1..c2a9393 100755 --- a/tests/pylint/runpylint.sh +++ b/tests/pylint/runpylint.sh @@ -77,6 +77,10 @@ export DISABLED_ERR_OPTIONS="--disable=E1103" # I0013 - Ignoring entire file (i.e., pylint: skip-file) export DISABLED_WARN_OPTIONS="--disable=W0110,W0123,W0141,W0142,W0511,W0603,W0613,W0614,I0011,I0012,I0013"
+# string is about half-deprecated, so add it to the list of deprecated modules +# and handle the valid cases with pragma comments. +export EXTRA_OPTIONS="--deprecated-modules=string,regsub,TERMIOS,Bastion,rexec" + usage () { echo "usage: `basename $0` [--strict] [--help] [files...]" exit $1
The string module is about half-deprecated, so we need to be careful about which parts of it we use. We don't use it too much, so warn about any use and filter out the correct ones with pylint pragmas.
Remove the uses of string.split, since strings can do that themselves nowadays and string.split is removed in python3, and string.letters, since this is also removed in python3 and we actually meant string.ascii_letters.
I like it.
- Chris
Knowing the exit code during exit is a fantasy. --- anaconda | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/anaconda b/anaconda index 19441b4..734dbff 100755 --- a/anaconda +++ b/anaconda @@ -119,7 +119,7 @@ def start_watched_pid(proc_name): os.close(writepipe) return childpid
-def exitHandler(rebootData, storage, exitCode=None): +def exitHandler(rebootData, storage): # stop and save coverage here b/c later the file system may be unavailable if coverage is not None: cov.stop() @@ -129,9 +129,6 @@ def exitHandler(rebootData, storage, exitCode=None): if flags.usevnc: vnc.shutdownServer()
- if exitCode: - anaconda.intf.shutdown() - if "nokill" in flags.cmdline: iutil.vtActivate(1) print("anaconda halting due to nokill flag.")
If we're rebooting, X or metacity exiting could throw an exception while everything shuts down, and we don't want that. --- anaconda | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/anaconda b/anaconda index 734dbff..e91ddca 100755 --- a/anaconda +++ b/anaconda @@ -120,6 +120,10 @@ def start_watched_pid(proc_name): return childpid
def exitHandler(rebootData, storage): + # Clear the list of watched PIDs. + global forever_pids + forever_pids = {} + # stop and save coverage here b/c later the file system may be unavailable if coverage is not None: cov.stop()
Do this by putting most of the global part of the anaconda script into a main() method and looking for ExitError thrown by main. Lightly rearrange some imports and variables to keep the ones that need to be global at the global level. --- anaconda | 154 ++++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 84 insertions(+), 70 deletions(-)
diff --git a/anaconda b/anaconda index e91ddca..d17cc1f 100755 --- a/anaconda +++ b/anaconda @@ -883,64 +883,7 @@ def cleanPStore(): from pyanaconda.iutil import dir_tree_map dir_tree_map("/sys/fs/pstore", os.unlink, files=True, dirs=False)
-if __name__ == "__main__": - # check if the CLI help is requested and return it at once, - # without importing random stuff and spamming stdout - if ("--help" in sys.argv) or ("-h" in sys.argv) or ("--version" in sys.argv): - # we skip the full logging initialisation, but we need to do at least - # this much (redirect any log messages to stdout) to get rid of the - # harmless but annoying "no handlers found" message on stdout - import logging - log = logging.getLogger("anaconda") - log.addHandler(logging.StreamHandler(stream=sys.stdout)) - parseArguments() - - print("Starting installer, one moment...") - - # Allow a file to be loaded as early as possible - try: - # pylint: disable=import-error,unused-import - import updates_disk_hook - except ImportError: - pass - - # this handles setting up updates for pypackages to minimize the set needed - setupPythonUpdates() - setupPythonPath() - - # init threading before Gtk can do anything and before we start using threads - # initThreading initializes the threadMgr instance, import it afterwards - from pyanaconda.threads import initThreading, AnacondaThread - initThreading() - from pyanaconda.threads import threadMgr - - from pyanaconda.i18n import _ - - from pyanaconda import constants - from pyanaconda.addons import collect_addon_paths - from pyanaconda import geoloc - - # do this early so we can set flags before initializing logging - from pyanaconda.flags import flags, can_touch_runtime_system - (opts, depr) = parseArguments(boot_cmdline=flags.cmdline) - - if opts.images: - flags.imageInstall = True - elif opts.dirinstall: - flags.dirInstall = True - - # Set up logging as early as possible. - import logging - from pyanaconda import anaconda_log - anaconda_log.init() - anaconda_log.logger.setupVirtio() - - from pyanaconda import network - network.setup_ifcfg_log() - - log = logging.getLogger("anaconda") - stdoutLog = logging.getLogger("anaconda.stdout") - +def main(): if os.geteuid() != 0: stdoutLog.error("anaconda must be run as root.") sys.exit(1) @@ -960,12 +903,6 @@ if __name__ == "__main__": # stdoutLog.warn("Boot argument '%s' is deprecated. " # "In the future, use 'inst.%s'.", arg, arg)
- # pull this in to get product name and versioning - from pyanaconda import product - - from pyanaconda import isys - - from pyanaconda import iutil
iutil.ipmi_report(constants.IPMI_STARTED)
@@ -981,7 +918,6 @@ if __name__ == "__main__": iutil.setTargetPhysicalRoot(root_path) iutil.setSysroot(root_path)
- from pyanaconda import vnc from pyanaconda import kickstart from pyanaconda import ntp from pyanaconda import keyboard @@ -994,8 +930,6 @@ if __name__ == "__main__": else: print("anaconda %s (pre-release) started." % verdesc)
- from pyanaconda.anaconda import Anaconda - anaconda = Anaconda() iutil.setup_translations()
# reset python's default SIGINT handler @@ -1095,6 +1029,7 @@ if __name__ == "__main__": os.system("udevadm control --env=ANACONDA=1")
# Collect all addon paths + from pyanaconda.addons import collect_addon_paths addon_paths = collect_addon_paths(constants.ADDON_PATHS)
# If we were given a kickstart file on the command line, parse (but do not @@ -1199,7 +1134,7 @@ if __name__ == "__main__": if len(url_parts) == 2: path = url_parts[1] elif len(url_parts) == 3: - fstype = url_parts[1] # XXX not used + fstype = url_parts[1] # pylint: disable=unused-variable path = url_parts[2]
ksdata.method.partition = device @@ -1256,7 +1191,6 @@ if __name__ == "__main__": # no kickstart or bootoption - use default localization.setup_locale(constants.DEFAULT_LANG, ksdata.lang)
- import blivet blivet.enable_installer_mode()
# now start the interface @@ -1283,7 +1217,7 @@ if __name__ == "__main__":
from pyanaconda.anaconda_argparse import name_path_pairs
- image_count = 0 + global image_count try: for (name, path) in name_path_pairs(opts.images): log.info("naming disk image '%s' '%s'", path, name) @@ -1340,6 +1274,7 @@ if __name__ == "__main__": # and other values or geoloc not being present as True use_geolocation = flags.cmdline.getbool('geoloc', True)
+ from pyanaconda import geoloc if use_geolocation: provider_id = constants.GEOLOC_DEFAULT_PROVIDER # check if a provider was specified by an option @@ -1369,4 +1304,83 @@ if __name__ == "__main__": anaconda._intf.setup(ksdata) anaconda._intf.run()
+if __name__ == "__main__": + # check if the CLI help is requested and return it at once, + # without importing random stuff and spamming stdout + if ("--help" in sys.argv) or ("-h" in sys.argv) or ("--version" in sys.argv): + # we skip the full logging initialisation, but we need to do at least + # this much (redirect any log messages to stdout) to get rid of the + # harmless but annoying "no handlers found" message on stdout + import logging + log = logging.getLogger("anaconda") + log.addHandler(logging.StreamHandler(stream=sys.stdout)) + parseArguments() + + print("Starting installer, one moment...") + + # Allow a file to be loaded as early as possible + try: + # pylint: disable=import-error,unused-import + import updates_disk_hook + except ImportError: + pass + + # this handles setting up updates for pypackages to minimize the set needed + setupPythonUpdates() + setupPythonPath() + + # Set up global imports + + # init threading before Gtk can do anything and before we start using threads + # initThreading initializes the threadMgr instance, import it afterwards + from pyanaconda.threads import initThreading, AnacondaThread + initThreading() + from pyanaconda.threads import threadMgr + + # do this early so we can set flags before initializing logging + from pyanaconda.flags import flags, can_touch_runtime_system + (opts, depr) = parseArguments(boot_cmdline=flags.cmdline) + if opts.images: + flags.imageInstall = True + elif opts.dirinstall: + flags.dirInstall = True + + # Set up logging as early as possible. + import logging + from pyanaconda import anaconda_log + anaconda_log.init() + anaconda_log.logger.setupVirtio() + + from pyanaconda import network + network.setup_ifcfg_log() + + log = logging.getLogger("anaconda") + stdoutLog = logging.getLogger("anaconda.stdout") + + from pyanaconda.i18n import _ + from pyanaconda import constants + from pyanaconda import vnc + from pyanaconda import iutil + from pyanaconda import isys + + # pull this in to get product name and versioning + from pyanaconda import product + + import blivet + + # Set up global variables + from pyanaconda.anaconda import Anaconda + anaconda = Anaconda() + + image_count = 0 + + try: + main() + except ExitError as e: + # X crashed or something + print("Anaconda is unable to continue: %s" % e.message) + iutil.ipmi_report(constants.IPMI_ABORTED) + # Re-raise the exception in case meh can still run + raise + # vim:tw=78:ts=4:et:sw=4
(as clumens noted, we already do ipmi stuff on exception via meh, so instead of all that other stuff just add ipmi stuff on exception before meh)
Send IMPI_ABORTED if anaconda crashes with an exception before the meh excepthook has been installed. --- anaconda | 1 + 1 file changed, 1 insertion(+)
diff --git a/anaconda b/anaconda index e91ddca..355b144 100755 --- a/anaconda +++ b/anaconda @@ -1072,6 +1072,7 @@ if __name__ == "__main__": if not flags.imageInstall and not flags.livecdInstall \ and not flags.dirInstall: def _earlyExceptionHandler(ty, value, traceback): + iutil.ipmi_report(constants.IPMI_ABORTED) iutil.vtActivate(1) return sys.__excepthook__(ty, value, traceback)
Send IMPI_ABORTED if anaconda crashes with an exception before the meh excepthook has been installed.
We are currently using IPMI_ABORTED to mean "the user chose to quit the installation" and IPMI_FAILED to mean "a traceback was hit". See the constant already in use in pyanaconda/exception.py.
- Chris
On 09/23/2014 11:24 AM, Chris Lumens wrote:
Send IMPI_ABORTED if anaconda crashes with an exception before the meh excepthook has been installed.
We are currently using IPMI_ABORTED to mean "the user chose to quit the installation" and IPMI_FAILED to mean "a traceback was hit". See the constant already in use in pyanaconda/exception.py.
Got it. Fixed locally.
On Tue, Sep 23, 2014 at 11:20:55AM -0400, David Shea wrote:
(as clumens noted, we already do ipmi stuff on exception via meh, so instead of all that other stuff just add ipmi stuff on exception before meh)
Send IMPI_ABORTED if anaconda crashes with an exception before the meh excepthook has been installed.
anaconda | 1 + 1 file changed, 1 insertion(+)
diff --git a/anaconda b/anaconda index e91ddca..355b144 100755 --- a/anaconda +++ b/anaconda @@ -1072,6 +1072,7 @@ if __name__ == "__main__": if not flags.imageInstall and not flags.livecdInstall \ and not flags.dirInstall:
These 3 checks could be replaced by can_touch_runtime_system
On 09/23/2014 12:15 PM, Brian C. Lane wrote:
On Tue, Sep 23, 2014 at 11:20:55AM -0400, David Shea wrote:
(as clumens noted, we already do ipmi stuff on exception via meh, so instead of all that other stuff just add ipmi stuff on exception before meh)
Send IMPI_ABORTED if anaconda crashes with an exception before the meh excepthook has been installed.
anaconda | 1 + 1 file changed, 1 insertion(+)
diff --git a/anaconda b/anaconda index e91ddca..355b144 100755 --- a/anaconda +++ b/anaconda @@ -1072,6 +1072,7 @@ if __name__ == "__main__": if not flags.imageInstall and not flags.livecdInstall \ and not flags.dirInstall:
These 3 checks could be replaced by can_touch_runtime_system
Can do.
The communicate() call is inside the lock, which isn't necessary. This way a running child doesn't block new child processes from being started from other threads. --- pyanaconda/iutil.py | 49 ++++++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index a849df8..254170a 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -127,37 +127,40 @@ def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_ou with program_log_lock: program_log.info("Running... %s", " ".join(argv))
- env = augmentEnv() - for var in env_prune: - env.pop(var, None) + env = augmentEnv() + for var in env_prune: + env.pop(var, None)
- try: - proc = subprocess.Popen(argv, - stdin=stdin, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - preexec_fn=chroot, cwd=root, env=env) + try: + proc = subprocess.Popen(argv, + stdin=stdin, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + preexec_fn=chroot, cwd=root, env=env)
- output_string = proc.communicate()[0] - if output_string: - if binary_output: - output_lines = [output_string] - else: - if output_string[-1] != "\n": - output_string = output_string + "\n" - output_lines = output_string.splitlines(True) + output_string = proc.communicate()[0] + if output_string: + if binary_output: + output_lines = [output_string] + else: + if output_string[-1] != "\n": + output_string = output_string + "\n" + output_lines = output_string.splitlines(True)
- for line in output_lines: - if log_output: + if log_output: + with program_log_lock: + for line in output_lines: program_log.info(line.strip())
- if stdout: - stdout.write(line) + if stdout: + stdout.write(output_string)
- except OSError as e: + except OSError as e: + with program_log_lock: program_log.error("Error running %s: %s", argv[0], e.strerror) - raise + raise
+ with program_log_lock: program_log.debug("Return code: %d", proc.returncode)
return (proc.returncode, output_string)
This shares the process starting code between _run_program and execReadlines. Additionally use startProgram to start the shell in execConsole instead of calling Popen directly.
For greater flexibility in process starting, more subprocess.Popen parameters are exposed by startProgram than by _run_program. startProgram is able to override stdout and stderr and accepts an additional preexec_fn. --- pyanaconda/iutil.py | 76 ++++++++++++++++++++---------------- tests/pyanaconda_tests/iutil_test.py | 40 +++++++++++++++++++ 2 files changed, 83 insertions(+), 33 deletions(-)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 254170a..a27cc21 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -99,16 +99,22 @@ def setSysroot(path): global _sysroot _sysroot = path
-def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_output=True, binary_output=False): - """ Run an external program, log the output and return it to the caller +def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + env_prune=None, **kwargs): + """ Start an external program and return the Popen object. + + The root argument is handled by passing a preexec_fn argument to + subprocess.Popen, but an additional preexec_fn can still be specified + and will be run. The user preexec_fn will be run last. + :param argv: The command to run and argument :param root: The directory to chroot to before running command. :param stdin: The file object to read stdin from. - :param stdout: Optional file object to write stdout and stderr to. + :param stdout: The file object to write stdout to. + :param stderr: The file object to write stderr to. :param env_prune: environment variable to remove before execution - :param log_output: whether to log the output of command - :param binary_output: whether to treat the output of command as binary data - :return: The return code of the command and the output + :param kwargs: Additional parameters to pass to subprocess.Popen + :return: A Popen object for the running command. """ if env_prune is None: env_prune = [] @@ -119,11 +125,19 @@ def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_ou if target_root == _root_path: target_root = getSysroot()
- def chroot(): + # Check for and save a preexec_fn argument + preexec_fn = kwargs.pop("preexec_fn", None) + + def preexec(): + # If a target root was specificed, chroot into it if target_root and target_root != '/': os.chroot(target_root) os.chdir("/")
+ # If the user specified an additional preexec_fn argument, run it + if preexec_fn is not None: + preexec_fn() + with program_log_lock: program_log.info("Running... %s", " ".join(argv))
@@ -131,12 +145,25 @@ def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_ou for var in env_prune: env.pop(var, None)
+ return subprocess.Popen(argv, + stdin=stdin, + stdout=stdout, + stderr=stderr, + preexec_fn=preexec, cwd=root, env=env, **kwargs) + +def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_output=True, binary_output=False): + """ Run an external program, log the output and return it to the caller + :param argv: The command to run and argument + :param root: The directory to chroot to before running command. + :param stdin: The file object to read stdin from. + :param stdout: Optional file object to write stdout and stderr to. + :param env_prune: environment variable to remove before execution + :param log_output: whether to log the output of command + :param binary_output: whether to treat the output of command as binary data + :return: The return code of the command and the output + """ try: - proc = subprocess.Popen(argv, - stdin=stdin, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - preexec_fn=chroot, cwd=root, env=env) + proc = startProgram(argv, root=root, stdin=stdin, env_prune=env_prune)
output_string = proc.communicate()[0] if output_string: @@ -229,36 +256,19 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None): Output from the file is not logged to program.log This returns a generator with the lines from the command until it has finished """ - if env_prune is None: - env_prune = [] - # Return the lines from stdout via a Queue def queue_lines(out, queue): for line in iter(out.readline, b''): queue.put(line.strip()) out.close()
- def chroot(): - if root and root != '/': - os.chroot(root) - os.chdir("/") - argv = [command] + argv - with program_log_lock: - program_log.info("Running... %s", " ".join(argv))
- env = augmentEnv() - for var in env_prune: - env.pop(var, None) try: - proc = subprocess.Popen(argv, - stdin=stdin, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - bufsize=1, - preexec_fn=chroot, cwd=root, env=env) + proc = startProgram(argv, root=root, stdin=stdin, env_prune=env_prune, bufsize=1) except OSError as e: - program_log.error("Error running %s: %s", argv[0], e.strerror) + with program_log_lock: + program_log.error("Error running %s: %s", argv[0], e.strerror) raise
q = Queue() @@ -282,7 +292,7 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None): ## Run a shell. def execConsole(): try: - proc = subprocess.Popen(["/bin/sh"]) + proc = startProgram(["/bin/sh"], stdout=None, stderr=None) proc.wait() except OSError as e: raise RuntimeError("Error running /bin/sh: " + e.strerror) diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index 358a2d7..b49d5ba 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -268,6 +268,46 @@ exit 0 finally: signal.signal(signal.SIGHUP, old_HUP_handler)
+ def start_program_preexec_fn_test(self): + """Test passing preexec_fn to startProgram.""" + + marker_text = "yo wassup man" + # Create a temporary file that will be written before exec + with tempfile.NamedTemporaryFile() as testfile: + + # Write something to testfile to show this method was run + def preexec(): + # Open a copy of the file here since close_fds has already closed the descriptor + testcopy = open(testfile.name, 'w') + testcopy.write(marker_text) + testcopy.close() + + with timer(5): + # Start a program that does nothing, with a preexec_fn + proc = iutil.startProgram(["/bin/true"], preexec_fn=preexec) + proc.communicate() + + # Rewind testfile and look for the text + testfile.seek(0, os.SEEK_SET) + self.assertEqual(testfile.read(), marker_text) + + def start_program_stdout_test(self): + """Test redirecting stdout with startProgram.""" + + marker_text = "yo wassup man" + # Create a temporary file that will be written by the program + with tempfile.NamedTemporaryFile() as testfile: + # Open a new copy of the file so that the child doesn't close and + # delete the NamedTemporaryFile + stdout = open(testfile.name, 'w') + with timer(5): + proc = iutil.startProgram(["/bin/echo", marker_text], stdout=stdout) + proc.communicate() + + # Rewind testfile and look for the text + testfile.seek(0, os.SEEK_SET) + self.assertEqual(testfile.read().strip(), marker_text) + class MiscTests(unittest.TestCase): def get_dir_size_test(self): """Test the getDirSize."""
When set, reset_handlers will reset all SIG_IGN handlers to SIG_DFL after forking the new process. This includes the SIGPIPE handler set by Python. The option is set by default, since pretty much the only time we want a SIG_IGN to be inherited is for X's weird startup signalling.
Add a test to make sure the option works. --- pyanaconda/iutil.py | 19 +++++++++++++++---- tests/pyanaconda_tests/iutil_test.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index a27cc21..1e5fed9 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -35,6 +35,7 @@ from threading import Thread from Queue import Queue, Empty from urllib import quote, unquote import gettext +import signal
from pyanaconda.flags import flags from pyanaconda.constants import DRACUT_SHUTDOWN_EJECT, TRANSLATIONS_UPDATE_DIR, UNSUPPORTED_HW @@ -100,12 +101,13 @@ def setSysroot(path): _sysroot = path
def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, - env_prune=None, **kwargs): + env_prune=None, reset_handlers=True, **kwargs): """ Start an external program and return the Popen object.
- The root argument is handled by passing a preexec_fn argument to - subprocess.Popen, but an additional preexec_fn can still be specified - and will be run. The user preexec_fn will be run last. + The root and reset_handlers arguments are handled by passing a + preexec_fn argument to subprocess.Popen, but an additional preexec_fn + can still be specified and will be run. The user preexec_fn will be run + last.
:param argv: The command to run and argument :param root: The directory to chroot to before running command. @@ -113,6 +115,7 @@ def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subp :param stdout: The file object to write stdout to. :param stderr: The file object to write stderr to. :param env_prune: environment variable to remove before execution + :param reset_handlers: whether to reset to SIG_DFL any signal handlers set to SIG_IGN :param kwargs: Additional parameters to pass to subprocess.Popen :return: A Popen object for the running command. """ @@ -134,6 +137,14 @@ def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subp os.chroot(target_root) os.chdir("/")
+ # Signal handlers set to SIG_IGN persist across exec. Reset + # these to SIG_DFL if requested. In particular this will include the + # SIGPIPE handler set by python. + if reset_handlers: + for signum in range(1, signal.NSIG): + if signal.getsignal(signum) == signal.SIG_IGN: + signal.signal(signum, signal.SIG_DFL) + # If the user specified an additional preexec_fn argument, run it if preexec_fn is not None: preexec_fn() diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index b49d5ba..b048f9c 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -308,6 +308,37 @@ exit 0 testfile.seek(0, os.SEEK_SET) self.assertEqual(testfile.read().strip(), marker_text)
+ def start_program_reset_handlers_test(self): + """Test the reset_handlers parameter of startProgram.""" + + with tempfile.NamedTemporaryFile() as testscript: + testscript.write("""#!/bin/sh +# Just hang out and do nothing, forever +while true ; do sleep 1 ; done +""") + testscript.flush() + + # Start a program with reset_handlers + proc = iutil.startProgram(["/bin/sh", testscript.name]) + + with timer(5): + # Kill with SIGPIPE and check that the python's SIG_IGN was not inheritted + # The process should die on the signal. + proc.send_signal(signal.SIGPIPE) + proc.communicate() + self.assertEqual(proc.returncode, -(signal.SIGPIPE)) + + # Start another copy without reset_handlers + proc = iutil.startProgram(["/bin/sh", testscript.name], reset_handlers=False) + + with timer(5): + # Kill with SIGPIPE, then SIGTERM, and make sure SIGTERM was the one + # that worked. + proc.send_signal(signal.SIGPIPE) + proc.terminate() + proc.communicate() + self.assertEqual(proc.returncode, -(signal.SIGTERM)) + class MiscTests(unittest.TestCase): def get_dir_size_test(self): """Test the getDirSize."""
I don't think we ever want anything above stderr to make it to a child process. --- pyanaconda/iutil.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 1e5fed9..999b4ec 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -160,6 +160,7 @@ def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subp stdin=stdin, stdout=stdout, stderr=stderr, + close_fds=True, preexec_fn=preexec, cwd=root, env=env, **kwargs)
def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_output=True, binary_output=False):
Remove the thread and queue between execReadlines and the process it starts. Instead of reading child process output into an unbounded buffer, have the child block until we are ready to read input, and kill the child when we are done reading input. Return an iterator object instead of a generator so that we can kill the child on __del__.
Add a test for whether the child is killed when the iterator is unreferenced. --- pyanaconda/iutil.py | 61 ++++++++++-------- tests/pyanaconda_tests/iutil_test.py | 118 +++++++++++++++++++++-------------- 2 files changed, 108 insertions(+), 71 deletions(-)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 999b4ec..97b9f35 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -31,8 +31,6 @@ import string import tempfile import types import re -from threading import Thread -from Queue import Queue, Empty from urllib import quote, unquote import gettext import signal @@ -266,13 +264,42 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None): :param env_prune: environment variable to remove before execution
Output from the file is not logged to program.log - This returns a generator with the lines from the command until it has finished + This returns an iterator with the lines from the command until it has finished """ - # Return the lines from stdout via a Queue - def queue_lines(out, queue): - for line in iter(out.readline, b''): - queue.put(line.strip()) - out.close() + + class ExecLineReader(object): + """Iterator class for returning lines from a process and cleaning + up the process when the output is no longer needed. + """ + + def __init__(self, proc, argv): + self._proc = proc + self._argv = argv + + def __iter__(self): + return self + + def __del__(self): + # See if the process is still running + if self._proc.poll() is None: + # Stop the process and ignore any problems that might arise + try: + self._proc.terminate() + except OSError: + pass + + def next(self): + # Read the next line, blocking if a line is not yet available + line = self._proc.stdout.readline() + if line == '': + # Output finished, check for the process dying unexpectedly + # and stop the iteration + if self._proc.poll() is not None: + if os.WIFSIGNALED(self._proc.returncode): + raise OSError("process '%s' was killed" % self._argv) + raise StopIteration + + return line.strip()
argv = [command] + argv
@@ -283,23 +310,7 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None): program_log.error("Error running %s: %s", argv[0], e.strerror) raise
- q = Queue() - t = Thread(target=queue_lines, args=(proc.stdout, q)) - t.daemon = True # thread dies with the program - t.start() - - while True: - try: - line = q.get(timeout=.1) - yield line - q.task_done() - except Empty: - if proc.poll() is not None: - if os.WIFSIGNALED(proc.returncode): - raise OSError("process '%s' was killed" % argv) - break - q.join() - + return ExecLineReader(proc, argv)
## Run a shell. def execConsole(): diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index b048f9c..4d28606 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -21,7 +21,6 @@
from pyanaconda import iutil import unittest -import types import os import tempfile import signal @@ -103,12 +102,10 @@ class RunProgramTests(unittest.TestCase): # test some lines are returned self.assertGreater(len(list(iutil.execReadlines("ls", ["--help"]))), 0)
- # check that it always returns a generator for both + # check that it always returns an iterator for both # if there is some output and if there isn't any - self.assertIsInstance(iutil.execReadlines("ls", ["--help"]), - types.GeneratorType) - self.assertIsInstance(iutil.execReadlines("true", []), - types.GeneratorType) + self.assertTrue(hasattr(iutil.execReadlines("ls", ["--help"]), "__iter__")) + self.assertTrue(hasattr(iutil.execReadlines("true", []), "__iter__"))
def exec_readlines_test_normal_output(self): """Test the output of execReadlines.""" @@ -124,11 +121,11 @@ exit 0 testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(StopIteration, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(StopIteration, rl_iterator.next)
# Test output with no end of line with tempfile.NamedTemporaryFile() as testscript: @@ -141,11 +138,11 @@ exit 0 testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(StopIteration, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(StopIteration, rl_iterator.next)
def exec_readlines_test_exits(self): """Test execReadlines in different child exit situations.""" @@ -163,11 +160,11 @@ exit 1 testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(OSError, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(OSError, rl_iterator.next)
# Test exit on signal with tempfile.NamedTemporaryFile() as testscript: @@ -180,11 +177,11 @@ kill -TERM $$ testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(OSError, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(OSError, rl_iterator.next)
# Repeat the above two tests, but exit before a final newline with tempfile.NamedTemporaryFile() as testscript: @@ -197,11 +194,11 @@ exit 1 testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(OSError, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(OSError, rl_iterator.next)
with tempfile.NamedTemporaryFile() as testscript: testscript.write("""#!/bin/sh @@ -213,11 +210,11 @@ kill -TERM $$ testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(OSError, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(OSError, rl_iterator.next)
def exec_readlines_test_signals(self): """Test execReadlines and signal receipt.""" @@ -236,11 +233,11 @@ exit 0 testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(StopIteration, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(StopIteration, rl_iterator.next) finally: signal.signal(signal.SIGHUP, old_HUP_handler)
@@ -260,11 +257,11 @@ exit 0 testscript.flush()
with timer(5): - rl_generator = iutil.execReadlines("/bin/sh", [testscript.name]) - self.assertEqual(rl_generator.next(), "one") - self.assertEqual(rl_generator.next(), "two") - self.assertEqual(rl_generator.next(), "three") - self.assertRaises(StopIteration, rl_generator.next) + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + self.assertEqual(rl_iterator.next(), "one") + self.assertEqual(rl_iterator.next(), "two") + self.assertEqual(rl_iterator.next(), "three") + self.assertRaises(StopIteration, rl_iterator.next) finally: signal.signal(signal.SIGHUP, old_HUP_handler)
@@ -339,6 +336,35 @@ while true ; do sleep 1 ; done proc.communicate() self.assertEqual(proc.returncode, -(signal.SIGTERM))
+ def exec_readlines_auto_kill_test(self): + """Test execReadlines with reading only part of the output""" + + with tempfile.NamedTemporaryFile() as testscript: + testscript.write("""#!/bin/sh +# Output forever +while true; do +echo hey +done +""") + testscript.flush() + + with timer(5): + rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name]) + + # Save the process context + proc = rl_iterator._proc + + # Read two lines worth + self.assertEqual(rl_iterator.next(), "hey") + self.assertEqual(rl_iterator.next(), "hey") + + # Delete the iterator and wait for the process to be killed + del rl_iterator + proc.communicate() + + # Check that the process is gone + self.assertIsNotNone(proc.poll()) + class MiscTests(unittest.TestCase): def get_dir_size_test(self): """Test the getDirSize."""
stderr will still be logged, but callers that don't care about what's printed to stderr can ignore it. --- pyanaconda/iutil.py | 30 ++++++++++++++++++++++++------ tests/pyanaconda_tests/iutil_test.py | 19 +++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 97b9f35..2ee0ebb 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -161,21 +161,29 @@ def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subp close_fds=True, preexec_fn=preexec, cwd=root, env=env, **kwargs)
-def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_output=True, binary_output=False): +def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_output=True, + binary_output=False, filter_stderr=False): """ Run an external program, log the output and return it to the caller :param argv: The command to run and argument :param root: The directory to chroot to before running command. :param stdin: The file object to read stdin from. - :param stdout: Optional file object to write stdout and stderr to. + :param stdout: Optional file object to write the output to. :param env_prune: environment variable to remove before execution :param log_output: whether to log the output of command :param binary_output: whether to treat the output of command as binary data + :param filter_stderr: whether to exclude the contents of stderr from the returned output :return: The return code of the command and the output """ try: - proc = startProgram(argv, root=root, stdin=stdin, env_prune=env_prune) + if filter_stderr: + stderr = subprocess.PIPE + else: + stderr = subprocess.STDOUT
- output_string = proc.communicate()[0] + proc = startProgram(argv, root=root, stdin=stdin, stdout=subprocess.PIPE, stderr=stderr, + env_prune=env_prune) + + (output_string, err_string) = proc.communicate() if output_string: if binary_output: output_lines = [output_string] @@ -192,6 +200,14 @@ def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_ou if stdout: stdout.write(output_string)
+ # If stderr was filtered, log it separately + if filter_stderr and err_string and log_output: + err_lines = err_string.splitlines(True) + + with program_log_lock: + for line in err_lines: + program_log.info(line.strip()) + except OSError as e: with program_log_lock: program_log.error("Error running %s: %s", argv[0], e.strerror) @@ -234,13 +250,14 @@ def execWithRedirect(command, argv, stdin=None, stdout=None, return _run_program(argv, stdin=stdin, stdout=stdout, root=root, env_prune=env_prune, log_output=log_output, binary_output=binary_output)[0]
-def execWithCapture(command, argv, stdin=None, root='/', log_output=True): +def execWithCapture(command, argv, stdin=None, root='/', log_output=True, filter_stderr=False): """ Run an external program and capture standard out and err. :param command: The command to run :param argv: The argument list :param stdin: The file object to read stdin from. :param root: The directory to chroot to before running command. :param log_output: Whether to log the output of command + :param filter_stderr: Whether stderr should be excluded from the returned output :return: The output of the command """ if flags.testing: @@ -249,7 +266,8 @@ def execWithCapture(command, argv, stdin=None, root='/', log_output=True): return ""
argv = [command] + argv - return _run_program(argv, stdin=stdin, root=root, log_output=log_output)[1] + return _run_program(argv, stdin=stdin, root=root, log_output=log_output, + filter_stderr=filter_stderr)[1]
def execReadlines(command, argv, stdin=None, root='/', env_prune=None): """ Execute an external command and return the line output of the command diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index 4d28606..dde3129 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -93,6 +93,25 @@ class RunProgramTests(unittest.TestCase): # check no output is returned self.assertEqual(len(iutil.execWithCapture('true', [])), 0)
+ def exec_with_capture_no_stderr_test(self): + """Test execWithCapture with no stderr""" + + with tempfile.NamedTemporaryFile() as testscript: + testscript.write("""#!/bin/sh +echo "output" +echo "error" >&2 +""") + testscript.flush() + + # check that only the output is captured + self.assertEqual( + iutil.execWithCapture("/bin/sh", [testscript.name], filter_stderr=True), + "output\n") + + # check that both output and error are captured + self.assertEqual(iutil.execWithCapture("/bin/sh", [testscript.name]), + "output\nerror\n") + def exec_readlines_test(self): """Test execReadlines."""
This way parent processes don't try waiting for output that will never come. --- pyanaconda/isys/auditd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/pyanaconda/isys/auditd.c b/pyanaconda/isys/auditd.c index a75289c..dfe6ad4 100644 --- a/pyanaconda/isys/auditd.c +++ b/pyanaconda/isys/auditd.c @@ -107,6 +107,11 @@ int audit_daemonize(void) { if (child < 0) return -1;
+ /* Close stdin and friends */ + close(STDIN_FILENO); + close(STDOUT_FILENO); + close(STDERR_FILENO); + if ((fd = open("/proc/self/oom_score_adj", O_RDWR)) >= 0) { write(fd, "-1000", 5); close(fd);
On Sun, 2014-09-21 at 15:37 -0400, David Shea wrote:
This way parent processes don't try waiting for output that will never come.
pyanaconda/isys/auditd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/pyanaconda/isys/auditd.c b/pyanaconda/isys/auditd.c index a75289c..dfe6ad4 100644 --- a/pyanaconda/isys/auditd.c +++ b/pyanaconda/isys/auditd.c @@ -107,6 +107,11 @@ int audit_daemonize(void) { if (child < 0) return -1;
- /* Close stdin and friends */
- close(STDIN_FILENO);
- close(STDOUT_FILENO);
- close(STDERR_FILENO);
Any chance we could use the 'daemon()' glibc function here?
On 09/30/2014 12:40 PM, Vratislav Podzimek wrote:
On Sun, 2014-09-21 at 15:37 -0400, David Shea wrote:
This way parent processes don't try waiting for output that will never come.
pyanaconda/isys/auditd.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/pyanaconda/isys/auditd.c b/pyanaconda/isys/auditd.c index a75289c..dfe6ad4 100644 --- a/pyanaconda/isys/auditd.c +++ b/pyanaconda/isys/auditd.c @@ -107,6 +107,11 @@ int audit_daemonize(void) { if (child < 0) return -1;
- /* Close stdin and friends */
- close(STDIN_FILENO);
- close(STDOUT_FILENO);
- close(STDERR_FILENO);
Any chance we could use the 'daemon()' glibc function here?
Yeah, probably
Change the process watching methods to use Popen objects. Convert X and metacity startup to use startProgram and watchProcess. This also moves startMetacityWM into doStartupX11Actions because there isn't much left of the former. --- anaconda | 166 +++++------------------------------ pyanaconda/iutil.py | 94 ++++++++++++++++++++ tests/pyanaconda_tests/iutil_test.py | 19 ++++ 3 files changed, 136 insertions(+), 143 deletions(-)
diff --git a/anaconda b/anaconda index d17cc1f..f9807cd 100755 --- a/anaconda +++ b/anaconda @@ -44,85 +44,11 @@ if ("debug=1" in proc_cmdline) or ("debug" in proc_cmdline): cov.start()
-import atexit, sys, os, time, subprocess, signal, errno - -# Install a global SIGCHLD handler to keep track of things that should be -# running for as long as anaconda does. The dictionary is of the form -# {pid: name, ...}. The handler will raise ExitError (defined below), so if -# not caught a SIGCHLD from a watched process will halt anaconda. -forever_pids = {} - -class ExitError(RuntimeError): - pass - -def sigchld_handler(num, frame): - # Check whether anything in the list of processes being watched has - # exited. We don't want to call waitpid(-1), since that would break - # anything else using wait/waitpid (like the subprocess module). - exited_pids = [] - exn_message = [] - - for child_pid in forever_pids: - try: - pid_result, status = os.waitpid(child_pid, os.WNOHANG) - except OSError as e: - if e.errno == errno.ECHILD: - continue - - if pid_result: - proc_name = forever_pids[child_pid] - exited_pids.append(child_pid) - - if os.WIFEXITED(status): - status_str = "with status %s" % os.WEXITSTATUS(status) - elif os.WIFSIGNALED(status): - status_str = "on signal %s" % os.WTERMSIG(status) - else: - status_str = "with unknown status code %s" % status - - exn_message.append("%s exited %s" % (proc_name, status_str)) - - for child_pid in exited_pids: - del forever_pids[child_pid] - - if exn_message: - raise ExitError(", ".join(exn_message)) - -signal.signal(signal.SIGCHLD, sigchld_handler) - -# Fork a new process and add it to forever_pids. The return values are the -# the same as os.fork, but neither the parent nor the child will return -# until the process is watched. -def start_watched_pid(proc_name): - readpipe, writepipe = os.pipe() - childpid = os.fork() - if childpid == 0: - # No need for the write pipe in the child - os.close(writepipe) - - # Wait for the parent to signal that it's ready to return - os.read(readpipe, 1) - - # Ready to go - os.close(readpipe) - return childpid - else: - # No need for the read pipe in the parent - os.close(readpipe) - - # Add the pid to the list of watched pids - forever_pids[childpid] = proc_name - - # Signal to the child that we're ready to return - # D is for Done - os.write(writepipe, 'D') - os.close(writepipe) - return childpid +import atexit, sys, os, time, subprocess, signal
def exitHandler(rebootData, storage): # Clear the list of watched PIDs. - global forever_pids - forever_pids = {} + iutil.unwatchAllProcesses()
# stop and save coverage here b/c later the file system may be unavailable if coverage is not None: @@ -196,56 +122,29 @@ def startX11(): if x11_started[0]: return log.error("Timeout trying to start the X server") - raise ExitError("Timeout trying to start the X server") + raise iutil.ExitError("Timeout trying to start the X server") + + # preexec_fn to add the SIGUSR1 handler in the child + def sigusr1_preexec(): + signal.signal(signal.SIGUSR1, signal.SIG_IGN)
try: old_sigusr1_handler = signal.signal(signal.SIGUSR1, sigusr1_handler) old_sigalrm_handler = signal.signal(signal.SIGALRM, sigalrm_handler)
- childpid = start_watched_pid("Xorg") + # Open /dev/tty5 for stdout and stderr redirects + xfd = open("/dev/tty5", "a")
- if not childpid: - # after this point the method should never return (or throw an exception - # outside) - try: - # dup /dev/tty5 to stdout and stderr - xfd = os.open("/dev/tty5", os.O_WRONLY | os.O_APPEND) - os.dup2(xfd, sys.stdout.fileno()) - os.dup2(xfd, sys.stderr.fileno()) - - # Close all other file descriptors - try: - maxfd = os.sysconf("SC_OPEN_MAX") - except ValueError: - maxfd = 1024 - - os.closerange(3, maxfd) - - # Replace the SIGUSR1 handler with SIG_IGN as described above - signal.signal(signal.SIGUSR1, signal.SIG_IGN) - - # Run it - os.execlp("Xorg", "Xorg", "-br", - "-logfile", "/tmp/X.log", - ":1", "vt6", "-s", "1440", "-ac", - "-nolisten", "tcp", "-dpi", "96", - "-noreset") - - # We should never get here - raise OSError(0, "Unable to exec") - except BaseException as e: - # catch all possible exceptions - # Reopen the logger in case it was closed by closerange. - # Use a different name because assigning to variables across a - # fork confuses the hell out of python. - child_log = logging.getLogger("anaconda") - child_log.error("Problems running Xorg: %s", e) - os._exit(1) - - # Parent process # Start the timer signal.alarm(60)
+ childproc = iutil.startProgram(["Xorg", "-br", "-logfile", "/tmp/X.log", + ":1", "vt6", "-s", "1440", "-ac", + "-nolisten", "tcp", "-dpi", "96", + "-noreset"], stdout=xfd, stderr=xfd, + preexec_fn=sigusr1_preexec) + iutil.watchProcess(childproc, "Xorg") + # Wait for SIGUSR1 while not x11_started[0]: signal.pause() @@ -258,28 +157,17 @@ def startX11(): signal.signal(signal.SIGUSR1, old_sigusr1_handler) signal.signal(signal.SIGALRM, old_sigalrm_handler)
-def startMetacityWM(): +# function to handle X startup special issues for anaconda +def doStartupX11Actions(): + """Start window manager""" + # When metacity actually connects to the X server is unknowable, but # fortunately it doesn't matter. metacity does not need to be the first # connection to Xorg, and if anaconda starts up before metacity, metacity # will just take over and maximize the window and make everything right, # fingers crossed. - - childpid = start_watched_pid("metacity") - if not childpid: - # after this point the method should never return (or throw an exception - # outside) - try: - returncode = iutil.execWithRedirect('metacity', ["--display", ":1", "--sm-disable"]) - except BaseException as e: - # catch all possible exceptions - log.error("Problems running the window manager: %s", e) - os._exit(1) - - log.info("The window manager has terminated.") - os._exit(returncode) - - return childpid + childproc = iutil.startProgram(["metacity", "--display", ":1", "--sm-disable"]) + iutil.watchProcess(childproc, "metacity")
def startAuditDaemon(): childpid = os.fork() @@ -293,14 +181,6 @@ def startAuditDaemon(): # auditd will turn into a daemon so catch the immediate child pid now: os.waitpid(childpid, 0)
-# function to handle X startup special issues for anaconda -def doStartupX11Actions(): - """Start window manager""" - - # now start up the window manager - wm_pid = startMetacityWM() - log.info("Starting window manager, pid %s.", wm_pid) - def set_x_resolution(runres): if runres and opts.display_mode == 'g' and not flags.usevnc: try: @@ -1376,7 +1256,7 @@ if __name__ == "__main__":
try: main() - except ExitError as e: + except iutil.ExitError as e: # X crashed or something print("Anaconda is unable to continue: %s" % e.message) iutil.ipmi_report(constants.IPMI_ABORTED) diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 2ee0ebb..e09b791 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -338,6 +338,100 @@ def execConsole(): except OSError as e: raise RuntimeError("Error running /bin/sh: " + e.strerror)
+# Dictionary of processes to watch in the form {pid: name, ...} +_forever_pids = {} +_watch_process_handler_set = False + +class ExitError(RuntimeError): + pass + +# Raise an error on process exit. The argument is a list of tuples +# of the form [(name, status), ...] with statuses in the subprocess +# format (>=0 is return codes, <0 is signal) +def _raise_exit_error(statuses): + exn_message = [] + + for proc_name, status in statuses: + if status >= 0: + status_str = "with status %s" % status + else: + status_str = "on signal %s" % -status + + exn_message.append("%s exited %s" % (proc_name, status_str)) + + raise ExitError(", ".join(exn_message)) + +# Signal handler used with watchProcess +def _sigchld_handler(num=None, frame=None): + # Check whether anything in the list of processes being watched has + # exited. We don't want to call waitpid(-1), since that would break + # anything else using wait/waitpid (like the subprocess module). + exited_pids = [] + exit_statuses = [] + + for child_pid in _forever_pids: + try: + pid_result, status = os.waitpid(child_pid, os.WNOHANG) + except OSError as e: + if e.errno == errno.ECHILD: + continue + + if pid_result: + proc_name = _forever_pids[child_pid] + exited_pids.append(child_pid) + + # Convert the wait-encoded status to the format used by subprocess + if os.WIFEXITED(status): + sub_status = os.WEXITSTATUS(status) + else: + sub_status = -os.WTERMSIG(status) + + exit_statuses.append((proc_name, sub_status)) + + for child_pid in exited_pids: + del _forever_pids[child_pid] + + if exit_statuses: + _raise_exit_error(exit_statuses) + +def watchProcess(proc, name): + """Watch for a process exit, and raise a ExitError when it does. + + This method installs a SIGCHLD signal handler and thus cannot be + used with the child_watch_add methods in GLib. Since the SIGCHLD + handler calls wait() on the watched process, this call cannot be + combined with Popen.wait() or Popen.communicate, and also doing + so wouldn't make a whole lot of sense. + + :param proc: The Popen object for the process + :param name: The name of the process + """ + global _watch_process_handler_set + + if not _watch_process_handler_set: + signal.signal(signal.SIGCHLD, _sigchld_handler) + _watch_process_handler_set = True + + # Add the PID to the dictionary + _forever_pids[proc.pid] = name + + # Check that the process didn't already exit + if proc.poll() is not None: + del _forever_pids[proc.pid] + _raise_exit_error([(name, proc.returncode)]) + +def unwatchProcess(proc): + """Unwatch a process watched by watchProcess. + + :param proc: The Popen object for the process. + """ + del _forever_pids[proc.pid] + +def unwatchAllProcesses(): + """Clear the watched process list.""" + global _forever_pids + _forever_pids = {} + def getDirSize(directory): """ Get the size of a directory and all its subdirectories. :param dir: The name of the directory to find the size of. diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index dde3129..a08e167 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -384,6 +384,25 @@ done # Check that the process is gone self.assertIsNotNone(proc.poll())
+ def watch_process_test(self): + """Test watchProcess""" + + def test_still_running(): + with timer(5): + # Run something forever so we can kill it + proc = iutil.startProgram(["/bin/sh", "-c", "while true; do sleep 1; done"]) + iutil.watchProcess(proc, "test1") + proc.kill() + # Wait for the SIGCHLD + signal.pause() + self.assertRaises(iutil.ExitError, test_still_running) + + # Make sure watchProcess checks that the process has not already exited + with timer(5): + proc = iutil.startProgram(["true"]) + proc.communicate() + self.assertRaises(iutil.ExitError, iutil.watchProcess, proc, "test2") + class MiscTests(unittest.TestCase): def get_dir_size_test(self): """Test the getDirSize."""
Move all the parts that set up signal handlers and wait for SIGUSR1 to iutil.startX. Use startX for starting Xvnc since it does the same thing as Xorg. --- anaconda | 56 +++++++++-------------------------------------------- pyanaconda/iutil.py | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ pyanaconda/vnc.py | 12 ++---------- 3 files changed, 63 insertions(+), 57 deletions(-)
diff --git a/anaconda b/anaconda index f9807cd..c2a17f2 100755 --- a/anaconda +++ b/anaconda @@ -107,55 +107,17 @@ def startSpiceVDAgent(): log.info("Started spice-vdagent.")
def startX11(): - # Start X11 with its USR1 handler set to ignore, which will make it send - # us SIGUSR1 if it succeeds. If it fails, catch SIGCHLD and bomb out. - # Use a list so the value can be modified from the handler - x11_started = [False] - def sigusr1_handler(num, frame): - log.debug("X server has signalled a successful start.") - x11_started[0] = True + # Open /dev/tty5 for stdout and stderr redirects + xfd = open("/dev/tty5", "a")
- # Fail after, let's say a minute, in case something weird happens - # and we don't receive SIGUSR1 - def sigalrm_handler(num, frame): - # Check that it didn't make it under the wire - if x11_started[0]: - return - log.error("Timeout trying to start the X server") - raise iutil.ExitError("Timeout trying to start the X server") + # Start Xorg and wait for it become ready + iutil.startX(["Xorg", "-br", "-logfile", "/tmp/X.log", + ":1", "vt6", "-s", "1440", "-ac", + "-nolisten", "tcp", "-dpi", "96", + "-noreset"], output_redirect=xfd)
- # preexec_fn to add the SIGUSR1 handler in the child - def sigusr1_preexec(): - signal.signal(signal.SIGUSR1, signal.SIG_IGN) - - try: - old_sigusr1_handler = signal.signal(signal.SIGUSR1, sigusr1_handler) - old_sigalrm_handler = signal.signal(signal.SIGALRM, sigalrm_handler) - - # Open /dev/tty5 for stdout and stderr redirects - xfd = open("/dev/tty5", "a") - - # Start the timer - signal.alarm(60) - - childproc = iutil.startProgram(["Xorg", "-br", "-logfile", "/tmp/X.log", - ":1", "vt6", "-s", "1440", "-ac", - "-nolisten", "tcp", "-dpi", "96", - "-noreset"], stdout=xfd, stderr=xfd, - preexec_fn=sigusr1_preexec) - iutil.watchProcess(childproc, "Xorg") - - # Wait for SIGUSR1 - while not x11_started[0]: - signal.pause() - - # Now that X is started, make $DISPLAY available - os.environ["DISPLAY"] = ":1" - finally: - # Put everything back where it was - signal.alarm(0) - signal.signal(signal.SIGUSR1, old_sigusr1_handler) - signal.signal(signal.SIGALRM, old_sigalrm_handler) + # Now that X is started, make $DISPLAY available + os.environ["DISPLAY"] = ":1"
# function to handle X startup special issues for anaconda def doStartupX11Actions(): diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index e09b791..f135d6f 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -161,6 +161,58 @@ def startProgram(argv, root='/', stdin=None, stdout=subprocess.PIPE, stderr=subp close_fds=True, preexec_fn=preexec, cwd=root, env=env, **kwargs)
+def startX(argv, output_redirect=None): + """ Start X and return once X is ready to accept connections. + + X11, if SIGUSR1 is set to SIG_IGN, will send SIGUSR1 to the parent + process once it is ready to accept client connections. This method + sets that up and waits for the signal or bombs out if nothing happens + for a minute. The process will also be added to the list of watched + processes. + + :param argv: The command line to run, as a list + :param output_redirect: file or file descriptor to redirect stdout and stderr to + """ + # Use a list so the value can be modified from the handler function + x11_started = [False] + def sigusr1_handler(num, frame): + log.debug("X server has signalled a successful start.") + x11_started[0] = True + + # Fail after, let's say a minute, in case something weird happens + # and we don't receive SIGUSR1 + def sigalrm_handler(num, frame): + # Check that it didn't make it under the wire + if x11_started[0]: + return + log.error("Timeout trying to start %s", argv[0]) + raise ExitError("Timeout trying to start %s" % argv[0]) + + # preexec_fn to add the SIGUSR1 handler in the child + def sigusr1_preexec(): + signal.signal(signal.SIGUSR1, signal.SIG_IGN) + + try: + old_sigusr1_handler = signal.signal(signal.SIGUSR1, sigusr1_handler) + old_sigalrm_handler = signal.signal(signal.SIGALRM, sigalrm_handler) + + # Start the timer + signal.alarm(60) + + childproc = startProgram(argv, stdout=output_redirect, stderr=output_redirect, + preexec_fn=sigusr1_preexec) + watchProcess(childproc, argv[0]) + + # Wait for SIGUSR1 + while not x11_started[0]: + signal.pause() + + finally: + # Put everything back where it was + signal.alarm(0) + signal.signal(signal.SIGUSR1, old_sigusr1_handler) + signal.signal(signal.SIGALRM, old_sigalrm_handler) + def _run_program(argv, root='/', stdin=None, stdout=None, env_prune=None, log_output=True, binary_output=False, filter_stderr=False): """ Run an external program, log the output and return it to the caller diff --git a/pyanaconda/vnc.py b/pyanaconda/vnc.py index 16a3faf..7eedbf8 100644 --- a/pyanaconda/vnc.py +++ b/pyanaconda/vnc.py @@ -221,21 +221,13 @@ class VncServer: "SecurityTypes=%s" % SecurityTypes, "rfbauth=%s" % rfbauth ]
try: - xvncp = subprocess.Popen(xvnccommand, stdout=self.openlogfile(), stderr=subprocess.STDOUT) + iutil.startX(xvnccommand, output_redirect=self.openlogfile()) except OSError: stdoutLog.critical("Could not start the VNC server. Aborting.") iutil.ipmi_report(constants.IPMI_ABORTED) sys.exit(1)
- # Lets give the xvnc time to initialize - time.sleep(1) - - # Make sure it hasn't blown up - if xvncp.poll() != None: - iutil.ipmi_report(constants.IPMI_ABORTED) - sys.exit(1) - else: - self.log.info(_("The VNC server is now running.")) + self.log.info(_("The VNC server is now running."))
# Lets tell the user what we are going to do. if self.vncconnecthost != "":
Replace direct uses of the subprocess module and os.system with our iutil wrappers to ensure consistency in process starting. Use execWithRedirect for things that just want to run a command to completion (most things), execWithCapture for the realm command that uses stdout, execConsole for a debug shel in the TUI, and startProgram for anything that manages the Popen object itself (nm-connection-editor, yelp) or does something complicated (vncconfig). --- anaconda | 30 +++++++++++------------------- pyanaconda/anaconda_log.py | 4 +++- pyanaconda/ihelp.py | 4 ++-- pyanaconda/kickstart.py | 32 +++++++++----------------------- pyanaconda/rescue.py | 4 +--- pyanaconda/ui/gui/spokes/network.py | 6 +++--- pyanaconda/ui/tui/spokes/askvnc.py | 5 +++-- pyanaconda/ui/tui/spokes/shell_spoke.py | 6 ++---- pyanaconda/vnc.py | 2 +- 9 files changed, 35 insertions(+), 58 deletions(-)
diff --git a/anaconda b/anaconda index c2a17f2..5aaaa3e 100755 --- a/anaconda +++ b/anaconda @@ -44,7 +44,7 @@ if ("debug=1" in proc_cmdline) or ("debug" in proc_cmdline): cov.start()
-import atexit, sys, os, time, subprocess, signal +import atexit, sys, os, time, signal
def exitHandler(rebootData, storage): # Clear the list of watched PIDs. @@ -84,7 +84,7 @@ def exitHandler(rebootData, storage): if not flags.imageInstall and not flags.livecdInstall \ and not flags.dirInstall: from pykickstart.constants import KS_SHUTDOWN, KS_WAIT - from pyanaconda.iutil import dracut_eject, get_mount_paths + from pyanaconda.iutil import dracut_eject, get_mount_paths, execWithRedirect
if flags.eject or rebootData.eject: for cdrom in storage.devicetree.getDevicesByType("cdrom"): @@ -92,11 +92,11 @@ def exitHandler(rebootData, storage): dracut_eject(cdrom.path)
if rebootData.action == KS_SHUTDOWN: - subprocess.Popen(["systemctl", "--no-wall", "poweroff"]) + execWithRedirect("systemctl", ["--no-wall", "poweroff"]) elif rebootData.action == KS_WAIT: - subprocess.Popen(["systemctl", "--no-wall", "halt"]) + execWithRedirect("systemctl", ["--no-wall", "halt"]) else: # reboot action is KS_REBOOT or None - subprocess.Popen(["systemctl", "--no-wall", "reboot"]) + execWithRedirect("systemctl", ["--no-wall", "reboot"])
def startSpiceVDAgent(): status = iutil.execWithRedirect("spice-vdagent", []) @@ -131,18 +131,6 @@ def doStartupX11Actions(): childproc = iutil.startProgram(["metacity", "--display", ":1", "--sm-disable"]) iutil.watchProcess(childproc, "metacity")
-def startAuditDaemon(): - childpid = os.fork() - if not childpid: - cmd = '/sbin/auditd' - try: - os.execl(cmd, cmd) - except OSError as e: - log.error("Error running the audit daemon: %s", e) - os._exit(0) - # auditd will turn into a daemon so catch the immediate child pid now: - os.waitpid(childpid, 0) - def set_x_resolution(runres): if runres and opts.display_mode == 'g' and not flags.usevnc: try: @@ -854,7 +842,11 @@ def main(): sys.excepthook = _earlyExceptionHandler
if can_touch_runtime_system("start audit daemon"): - startAuditDaemon() + # auditd will turn into a daemon and exit. Ignore startup errors + try: + iutil.execWithRedirect("/sbin/auditd", []) + except OSError: + pass
# setup links required for all install types for i in ("services", "protocols", "nsswitch.conf", "joe", "selinux", @@ -868,7 +860,7 @@ def main(): log.info("anaconda called with cmdline = %s", sys.argv) log.info("Default encoding = %s ", sys.getdefaultencoding())
- os.system("udevadm control --env=ANACONDA=1") + iutil.execWithRedirect("udevadm", ["control", "--env=ANACONDA=1"])
# Collect all addon paths from pyanaconda.addons import collect_addon_paths diff --git a/pyanaconda/anaconda_log.py b/pyanaconda/anaconda_log.py index 56ae9c7..498d64a 100644 --- a/pyanaconda/anaconda_log.py +++ b/pyanaconda/anaconda_log.py @@ -204,7 +204,9 @@ class AnacondaLog: message, category, filename, lineno, line))
def restartSyslog(self): - os.system("systemctl restart rsyslog.service") + # Import here instead of at the module level to avoid an import loop + from pyanaconda.iutil import execWithRedirect + execWithRedirect("systemctl", ["restart", "rsyslog.service"])
def updateRemote(self, remote_syslog): """Updates the location of remote rsyslogd to forward to. diff --git a/pyanaconda/ihelp.py b/pyanaconda/ihelp.py index cb02a84..5326048 100644 --- a/pyanaconda/ihelp.py +++ b/pyanaconda/ihelp.py @@ -21,11 +21,11 @@ Anaconda built-in help module """ import os -import subprocess
from pyanaconda.flags import flags from pyanaconda.localization import find_best_locale_match from pyanaconda.constants import DEFAULT_LANG +from pyanaconda.iutil import startProgram
import logging log = logging.getLogger("anaconda") @@ -124,7 +124,7 @@ def start_yelp(help_path): # under some extreme circumstances (placeholders missing) # the help path can be None and we need to prevent Popen # receiving None as an argument instead of a string - yelp_process = subprocess.Popen(["yelp", help_path or ""]) + yelp_process = startProgram(["yelp", help_path or ""])
def kill_yelp(): """Try to kill any existing yelp processes""" diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py index c0cb61c..a1c691b 100644 --- a/pyanaconda/kickstart.py +++ b/pyanaconda/kickstart.py @@ -41,7 +41,6 @@ from pyanaconda import iutil import os import os.path import tempfile -import subprocess from pyanaconda.flags import flags, can_touch_runtime_system from pyanaconda.constants import ADDON_PATHS, IPMI_ABORTED import shlex @@ -485,17 +484,13 @@ class Realm(commands.realm.F19_Realm): return
try: - argv = ["realm", "discover", "--verbose"] + \ + argv = ["discover", "--verbose"] + \ self.discover_options + [self.join_realm] - proc = subprocess.Popen(argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - output, stderr = proc.communicate() - # might contain useful information for users who use - # use the realm kickstart command - log.info("Realm discover stderr:\n%s", stderr) - except OSError as msg: + output = iutil.execWithCapture("realm", argv, filter_stderr=True) + except OSError: # TODO: A lousy way of propagating what will usually be # 'no such realm' - log.error("Error running realm %s: %s", argv, msg) + # The error message is logged by iutil return
# Now parse the output for the required software. First line is the @@ -531,21 +526,12 @@ class Realm(commands.realm.F19_Realm): pw_args + self.join_args rc = -1 try: - proc = subprocess.Popen(argv, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - stderr = proc.communicate()[1] - # might contain useful information for users who use - # use the realm kickstart command - log.info("Realm join stderr:\n%s", stderr) - rc = proc.returncode - except OSError as msg: - log.error("Error running %s: %s", argv, msg) + rc = iutil.execWithRedirect("realm", argv)[0] + except OSError: + pass
- if rc != 0: - log.error("Command failure: %s: %d", argv, rc) - return - - log.info("Joined realm %s", self.join_realm) + if rc == 0: + log.info("Joined realm %s", self.join_realm)
class ClearPart(commands.clearpart.F21_ClearPart): diff --git a/pyanaconda/rescue.py b/pyanaconda/rescue.py index 63bc0c1..a07a4ab 100644 --- a/pyanaconda/rescue.py +++ b/pyanaconda/rescue.py @@ -25,7 +25,6 @@ from pyanaconda import iutil import shutil import time import re -import subprocess
from snack import ButtonChoiceWindow, ListboxChoiceWindow,SnackScreen
@@ -190,8 +189,7 @@ def runShell(screen = None, msg=""): proc = None
if os.path.exists("/usr/bin/firstaidkit-qs"): - proc = subprocess.Popen(["/usr/bin/firstaidkit-qs"]) - proc.wait() + iutil.execWithRedirect("/usr/bin/firstaidkit-qs", [])
if proc is None or proc.returncode!=0: if os.path.exists("/bin/bash"): diff --git a/pyanaconda/ui/gui/spokes/network.py b/pyanaconda/ui/gui/spokes/network.py index 50b613e..271429c 100644 --- a/pyanaconda/ui/gui/spokes/network.py +++ b/pyanaconda/ui/gui/spokes/network.py @@ -41,6 +41,7 @@ from pyanaconda.ui.categories.system import SystemCategory from pyanaconda.ui.gui.hubs.summary import SummaryHub from pyanaconda.ui.gui.utils import gtk_call_once, escape_markup from pyanaconda.ui.common import FirstbootSpokeMixIn +from pyanaconda.iutil import startProgram
from pyanaconda import network from pyanaconda import nm @@ -48,7 +49,6 @@ from pyanaconda import nm from gi.repository import GLib, GObject, Pango, Gio, NetworkManager, NMClient import dbus import dbus.service -import subprocess import string from uuid import uuid4
@@ -545,7 +545,7 @@ class NetworkControlBox(GObject.GObject): log.info("network: configuring connection %s device %s ssid %s", uuid, devname, self.selected_ssid) self.kill_nmce(msg="Configure button clicked") - proc = subprocess.Popen(["nm-connection-editor", "--edit", "%s" % uuid]) + proc = startProgram(["nm-connection-editor", "--edit", "%s" % uuid]) self._running_nmce = proc
GLib.child_watch_add(proc.pid, self.on_nmce_exited, activate) @@ -632,7 +632,7 @@ class NetworkControlBox(GObject.GObject): def add_device(self, ty): log.info("network: adding device of type %s", ty) self.kill_nmce(msg="Add device button clicked") - proc = subprocess.Popen(["nm-connection-editor", "--create", "--type=%s" % ty]) + proc = startProgram(["nm-connection-editor", "--create", "--type=%s" % ty]) self._running_nmce = proc
GLib.child_watch_add(proc.pid, self.on_nmce_adding_exited) diff --git a/pyanaconda/ui/tui/spokes/askvnc.py b/pyanaconda/ui/tui/spokes/askvnc.py index 5d815e2..f9e087e 100644 --- a/pyanaconda/ui/tui/spokes/askvnc.py +++ b/pyanaconda/ui/tui/spokes/askvnc.py @@ -27,7 +27,8 @@ from pyanaconda.constants_text import INPUT_PROCESSED from pyanaconda.i18n import N_, _ from pyanaconda.ui.communication import hubQ from pyanaconda.ui.tui import exception_msg_handler -import getpass, subprocess +from pyanaconda.iutil import execWithRedirect +import getpass import sys
def exception_msg_handler_and_exit(event, data): @@ -104,7 +105,7 @@ class AskVNCSpoke(NormalTUISpoke): d = YesNoDialog(self.app, _(self.app.quit_message)) self.app.switch_screen_modal(d) if d.answer: - subprocess.Popen(["systemctl", "--no-wall", "reboot"]) + execWithRedirect("systemctl", ["--no-wall", "reboot"]) else: return key
diff --git a/pyanaconda/ui/tui/spokes/shell_spoke.py b/pyanaconda/ui/tui/spokes/shell_spoke.py index e0115ca..0037188 100644 --- a/pyanaconda/ui/tui/spokes/shell_spoke.py +++ b/pyanaconda/ui/tui/spokes/shell_spoke.py @@ -26,10 +26,9 @@ from pyanaconda.ui.tui.simpleline.widgets import TextWidget from pyanaconda.i18n import N_, _ from pyanaconda.constants import ANACONDA_ENVIRON from pyanaconda.flags import flags +from pyanaconda.iutil import execConsole from blivet import arch
-import subprocess - class ShellSpoke(NormalTUISpoke): title = N_("Shell") category = SystemCategory @@ -59,8 +58,7 @@ class ShellSpoke(NormalTUISpoke):
def prompt(self, args=None): # run shell instead of printing prompt and close window on shell exit - proc = subprocess.Popen(["bash", "--login"], shell=True, cwd="/") - proc.wait() + execConsole() self.close()
# suppress the prompt diff --git a/pyanaconda/vnc.py b/pyanaconda/vnc.py index 7eedbf8..a7e2072 100644 --- a/pyanaconda/vnc.py +++ b/pyanaconda/vnc.py @@ -158,7 +158,7 @@ class VncServer: vncconfigcommand = [self.root+"/usr/bin/vncconfig", "-display", ":%s"%self.display, "-connect", hostarg]
for _i in range(maxTries): - vncconfp = subprocess.Popen(vncconfigcommand, stdout=subprocess.PIPE, stderr=subprocess.PIPE) # vncconfig process + vncconfp = iutil.startProgram(vncconfigcommand, stdout=subprocess.PIPE, stderr=subprocess.PIPE) # vncconfig process err = vncconfp.communicate()[1]
if err == '':
I'm still digesting much of this, but I wanted to respond to one point while I was thinking about it:
- Should we catch Exception, instead? I don't think we do any ipmi stuff if something crashes and we get a meh handler, but I'm not 100% sure about that.
The line at the end of postWriteHook in pyanaconda/exception.py is supposed to take care of this case.
- Chris
On Sun, Sep 21, 2014, at 03:36 PM, David Shea wrote:
The overall goal is to ensure that any time we start a new process, it should be started with a clean set of signal handlers (in particular, make sure that SIGPIPE is set to SIG_DFL instead of the SIG_IGN set by python) and no file descriptors from the parent make it through exec() other than stdin, stdout and stderr.
Note GLib.spawn* does that by default, and with the new GSubprocess there are nicer async APIs on top. The problem going forward is still that only one thing can use SIGCHILD, so anaconda installing its own will break GLib's.
On 09/23/2014 11:09 AM, Colin Walters wrote:
On Sun, Sep 21, 2014, at 03:36 PM, David Shea wrote:
The overall goal is to ensure that any time we start a new process, it should be started with a clean set of signal handlers (in particular, make sure that SIGPIPE is set to SIG_DFL instead of the SIG_IGN set by python) and no file descriptors from the parent make it through exec() other than stdin, stdout and stderr.
Note GLib.spawn* does that by default, and with the new GSubprocess there are nicer async APIs on top. The problem going forward is still that only one thing can use SIGCHILD, so anaconda installing its own will break GLib's.
Yeah. We also have a problem with Gtk's signal handlers interfering with our python-installed handlers, so one of the things rattling on my whiteboard is to convert the signal handlers after Gtk.init, and I figured I'd convert the SIGCHLD watcher at that point, as well. I don't think that using GLib outside of the GUI portions of anaconda would be worthwhile.
On Sun, Sep 21, 2014, at 03:36 PM, David Shea wrote:
- None of this ever actually gets called, because gdk calls _exit(1)
(!!) when it loses the X connection.
That behavior is inherited from libX11 - every toolkit does it. The intention is to tie application lifecycle to that of the display.
Wasn't there a patch recently which just changed things to leave VNC open before reboot that addresses this?
* If we ask the Gtk to not freaking exit in a library call they probably won't
libdbus added an API for this (and libdbus does it too to provide lifecycle management for non-GUI components), but doesn't seem inconveivable for gtk, but I'm not a maintainer there myself.
On 09/23/2014 12:17 PM, Colin Walters wrote:
On Sun, Sep 21, 2014, at 03:36 PM, David Shea wrote:
- None of this ever actually gets called, because gdk calls _exit(1)
(!!) when it loses the X connection.
That behavior is inherited from libX11 - every toolkit does it. The intention is to tie application lifecycle to that of the display.
This is almost true. For example, qt calls exit(1) on an I/O error. The difference there is that 1) it doesn't call _exit, because skipping our shutdown handlers is seriously a jerk thing to do, and 2) there's at least a fixme comment showing they're aware that this behavior isn't ideal.
Wasn't there a patch recently which just changed things to leave VNC open before reboot that addresses this?
What if VNC crashes, is what I'm trying to get at.
On Tue, Sep 23, 2014, at 01:28 PM, David Shea wrote:
On 09/23/2014 12:17 PM, Colin Walters wrote:
On Sun, Sep 21, 2014, at 03:36 PM, David Shea wrote:
- None of this ever actually gets called, because gdk calls _exit(1)
(!!) when it loses the X connection.
That behavior is inherited from libX11 - every toolkit does it. The intention is to tie application lifecycle to that of the display.
This is almost true. For example, qt calls exit(1) on an I/O error. The difference there is that 1) it doesn't call _exit, because skipping our shutdown handlers is seriously a jerk thing to do, and 2) there's at least a fixme comment showing they're aware that this behavior isn't ideal.
Looks like this is:
https://bugzilla.gnome.org/show_bug.cgi?id=646338
Which is really i'd say a NetworkManager/nspr bug, but that's the current reason.
On 09/23/2014 03:48 PM, Colin Walters wrote:
On Tue, Sep 23, 2014, at 01:28 PM, David Shea wrote:
On 09/23/2014 12:17 PM, Colin Walters wrote:
On Sun, Sep 21, 2014, at 03:36 PM, David Shea wrote:
- None of this ever actually gets called, because gdk calls _exit(1) (!!) when it loses the X connection.That behavior is inherited from libX11 - every toolkit does it. The intention is to tie application lifecycle to that of the display.
This is almost true. For example, qt calls exit(1) on an I/O error. The difference there is that 1) it doesn't call _exit, because skipping our shutdown handlers is seriously a jerk thing to do, and 2) there's at least a fixme comment showing they're aware that this behavior isn't ideal.
Looks like this is:
https://bugzilla.gnome.org/show_bug.cgi?id=646338
Which is really i'd say a NetworkManager/nspr bug, but that's the current reason.
Fixing a bug in NetworkManager by bypassing NetworkManager's code in a library is the craziest thing I have heard all day.
And calling exit() would still only half-fix the problem. Being able to install custom callbacks or something would be ideal.
These all look pretty good to me, except for the few small comments and Colin's SIGCHLD concern.
anaconda-patches@lists.fedorahosted.org
-
Brian C. Lane -
Chris Lumens -
Colin Walters -
David Shea -
Vratislav Podzimek