[virt-tools-list] [H v2 2/2] cli: stop forking into the background

[Daniel P. Berrangé]

The behaviour whereby virt-manager forks into the background was added
way back in:

  commit 99c92b9471a6a55859307071aa4a0e712991f158
  Author: Daniel P. Berrange <berrange at redhat.com>
  Date:   Mon Sep 10 20:10:20 2007 -0400

    Refactor startup to drop controlling TTY, avoiding annoying SSH prompts

Most end users will launch virt-manager from the desktop which will fork
the app into the background already, preventing any console prompts.
When running from the command line, modern desktop environments will
have things setup up so that SSH key prompts are intercepted and
presented via a graphical window. Forking into the background was a hack
to deal with the case that the user launched virt-manager from the
terminal and connected to a remote host which didn't have SSH keys and
thus prompted for a password. At approx the same time as that was done,
libvirt gained a "no_tty=1" query parameter for its URIs to prevent SSH
prompts entirely. This feature is now used by virt-manager, so the main
rationale for forking into the background no longer exists.

Forking into the background causes extra pain for developers as warnings
that would otherwise appear on stderr get lost e.g.

  commit 24a8b66b35c92bed919a4a6beb7c7fb80e85b3b2
  Author: Daniel P. Berrangé <berrange at redhat.com>
  Date:   Wed Apr 4 14:35:40 2018 +0100

    avoid referencing ConnectError if it is None

    Currently it throws an exception at startup which is hidden unless you
    run with --no-fork

It also increases the complexity of command line argument parsing,
because some args need to be parsed before forking, while others would
be better handled after forking, using the GtkApplication framework.
Thus it is benefit to remove the forking code and just start "normally"
as any other GTK app would.

Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
---
 man/virt-manager.pod   |  7 +------
 tests/uitests/utils.py |  2 +-
 virt-manager           | 46 +---------------------------------------------
 3 files changed, 3 insertions(+), 52 deletions(-)

diff --git a/man/virt-manager.pod b/man/virt-manager.pod
index 387133f0..bab1ba51 100644
--- a/man/virt-manager.pod
+++ b/man/virt-manager.pod
@@ -41,12 +41,7 @@ Specify the hypervisor connection C<URI>
 =item B<--debug>
 
 List debugging output to the console (normally this is only logged in
-~/.cache/virt-manager/virt-manager.log). This function implies --no-fork.
-
-=item B<--no-fork>
-
-Don't fork C<virt-manager> off into the background: run it blocking the
-current terminal. Useful for seeing possible errors dumped to stdout/stderr.
+~/.cache/virt-manager/virt-manager.log)
 
 =item B<--show-DIALOG-WINDOW>
 
diff --git a/tests/uitests/utils.py b/tests/uitests/utils.py
index 4af451b3..cd5c4fa8 100644
--- a/tests/uitests/utils.py
+++ b/tests/uitests/utils.py
@@ -360,7 +360,7 @@ class VMMDogtailApp(object):
             cmd += ["-m", "coverage", "run", "--append",
                     "--omit", "/usr/*"]
         cmd += [os.path.join(os.getcwd(), "virt-manager"),
-                "--test-first-run", "--no-fork", "--connect", self.uri]
+                "--test-first-run", "--connect", self.uri]
         cmd += extra_opts
 
         self._proc = subprocess.Popen(cmd, stdout=stdout, stderr=stderr)
diff --git a/virt-manager b/virt-manager
index bc26be5c..451bbc30 100755
--- a/virt-manager
+++ b/virt-manager
@@ -49,12 +49,6 @@ def _show_startup_error(msg, details):
 
 
 def _import_gtk(leftovers):
-    # The never ending fork+gsettings problems now require us to
-    # import Gtk _after_ the fork. This creates a funny race, since we
-    # need to parse the command line arguments to know if we need to
-    # fork, but need to import Gtk before cli processing so it can
-    # handle --g-fatal-args. We strip out our flags first and pass the
-    # left overs to gtk
     origargv = sys.argv
     try:
         sys.argv = origargv[:1] + leftovers[:]
@@ -90,29 +84,6 @@ def _import_gtk(leftovers):
     return leftovers
 
 
-def drop_tty():
-    # We fork and setsid so that we drop the controlling
-    # tty. This prevents libvirt's SSH tunnels from prompting
-    # for user input if SSH keys/agent aren't configured.
-    if os.fork() != 0:
-        os._exit(0)  # pylint: disable=protected-access
-
-    os.setsid()
-
-
-def drop_stdio():
-    # This is part of the fork process described in drop_tty()
-    for fd in range(0, 2):
-        try:
-            os.close(fd)
-        except OSError:
-            pass
-
-    os.open(os.devnull, os.O_RDWR)
-    os.dup2(0, 1)
-    os.dup2(0, 2)
-
-
 def parse_commandline():
     epilog = ("Also accepts standard GTK arguments like --g-fatal-warnings")
     parser = argparse.ArgumentParser(usage="virt-manager [options]",
@@ -148,10 +119,8 @@ def parse_commandline():
     parser.add_argument("-c", "--connect", dest="uri",
         help="Connect to hypervisor at URI", metavar="URI")
     parser.add_argument("--debug", action="store_true",
-        help="Print debug output to stdout (implies --no-fork)",
+        help="Print debug output to stdout",
         default=False)
-    parser.add_argument("--no-fork", action="store_true",
-        help="Don't fork into background on startup")
 
     parser.add_argument("--show-domain-creator", action="store_true",
         help="Show 'New VM' wizard")
@@ -187,22 +156,9 @@ def main():
     if options.test_first_run:
         os.environ["GSETTINGS_BACKEND"] = "memory"
 
-    # Now we've got basic environment up & running we can fork
-    do_drop_stdio = False
-    if not options.no_fork and not options.debug:
-        drop_tty()
-        do_drop_stdio = True
-
-        # Ignore SIGHUP, otherwise a serial console closing drops the whole app
-        signal.signal(signal.SIGHUP, signal.SIG_IGN)
-
     leftovers = _import_gtk(leftovers)
     Gtk = globals()["Gtk"]
 
-    # Do this after the Gtk import so the user has a chance of seeing any error
-    if do_drop_stdio:
-        drop_stdio()
-
     if leftovers:
         raise RuntimeError("Unhandled command line options '%s'" % leftovers)
 
-- 
2.14.3