Use execv() to start selected session

Prevent vmchooser from lingering in the background, like a zombie,
haunting you with invisible windows. Also seems to be notably faster
when launching the openbox session.

10 files changed, 124 insertions, 104 deletions

diff --git a/src/dialog.cpp b/src/dialog.cpp
index 914448f..dbc4b8e 100644
--- a/src/dialog.cpp
+++ b/src/dialog.cpp
@@ -136,9 +136,11 @@ void Dialog::on_treeView_doubleClicked(const QModelIndex& index)
             return;
 
     // Check preconditions. The method might show error messages and return false
-    if (!s->canRun(true))
+    if (!s->prepareRun())
         return;
 
+    WindowManager::stopOwnInstance(true);
+
     // These two are up here in case run-virt cares...
     if (Config::isSet(Config::PVS)) {
         if (ui->PVS_checkbox->isChecked()) {
@@ -159,62 +161,41 @@ void Dialog::on_treeView_doubleClicked(const QModelIndex& index)
         setenv("VMCHOOSER_DISABLE_SCREENSAVER", "FALSE", 1);
     }
 
-    // Run session
-    if (s->run()) {
-        // Run session start script if the session could be initialized successfully
-        if (QFile::exists(SESSION_START_SCRIPT)) {
-            // Use the current environment variables and add the necessary
-            // information for the startUpScipt.
-            // export session information to the environment of the process to be exec'ed
-            setenv("SESSION_NAME", s->shortDescription().toUtf8(), 1);
-            setenv("SESSION_UUID", s->uuid().toUtf8(), 1);
-            setenv("SESSION_CMD", s->execCommand().toUtf8(), 1);
-            if (s->type() == Session::VSESSION) {
-               setenv("SESSION_TYPE", "VSESSION", 1);
-            } else if (s->type() == Session::XSESSION) {
-               setenv("SESSION_TYPE", "XSESSION", 1);
-            }
-            // Fork this process twice to detach
-            pid_t pid = fork();
-            if (pid == 0) {
-
-                // Close all filedescriptors
-                for (int i = 3; i < 1024; ++i)
-                    ::close(i);
-
-                // Change process group
-                setsid();
-
-                // Reopen standard pipes
-                freopen("/dev/null", "r", stdin);
-                freopen("/dev/null", "w", stdout);
-                freopen("/dev/null", "w", stderr);
-
-                pid = fork();
-                if (pid == 0) {
-                    // At this point we are executing as the 2nd child process
-                    // Replace this clone with the process we'd like to start
-                    char * const args[] = { strdup(VMCHOOSER_SESSION_START_SCRIPT), nullptr };
-                    execv(VMCHOOSER_SESSION_START_SCRIPT, args);
-                }
-                _exit(0); // Dont use exit hooks
-            }
-            // Wait and cleanup the intermediate process
-            waitpid(pid, nullptr, 0);
+    UserConfig::addLastSession(s->uuid().isEmpty() ? s->shortDescription() : s->uuid());
+    UserConfig::setLastTab(activeTab_);
+    UserConfig::setNewsHelpOpen(!ui->helpBox->isHidden());
+    UserConfig::sync();
+
+    QProcess *process = nullptr;
+    if (QFile::exists(SESSION_START_SCRIPT)) {
+        // Companion script
+        process = new QProcess;
+        QProcessEnvironment env = QProcessEnvironment::systemEnvironment();
+        env.insert("SESSION_NAME", s->shortDescription());
+        env.insert("SESSION_UUID", s->uuid());
+        env.insert("SESSION_CMD", s->execCommand());
+        if (s->type() == Session::VSESSION) {
+           env.insert("SESSION_TYPE", "VSESSION");
+        } else if (s->type() == Session::XSESSION) {
+           env.insert("SESSION_TYPE", "XSESSION");
         }
-        UserConfig::addLastSession(s->uuid().isEmpty() ? s->shortDescription() : s->uuid());
-        UserConfig::setLastTab(activeTab_);
-        UserConfig::setNewsHelpOpen(!ui->helpBox->isHidden());
-        centerTimer_->stop(); // Stop the auto-center/auto-quit timer, so we don't kill the session :>
-        setVisible(false);
-        // Stopping the window manager has to be done after hiding our window,
-        // or it might still be shown in some Xsessions (e.g. gnome-shell).
-        WindowManager::stopOwnInstance();
-    } else {
-        QMessageBox::critical(
-                    this, trUtf8("vmchooser"),
-                    trUtf8("Vmchooser failed to run the selected session!"));
+        process->setProcessEnvironment(env);
+        process->start(SESSION_START_SCRIPT);
+        process->closeReadChannel(QProcess::StandardError);
+        process->closeReadChannel(QProcess::StandardOutput);
+        process->closeWriteChannel();
     }
+
+    // Run session
+    s->run();
+    // Should not return on success
+    if (process != nullptr) {
+        process->terminate();
+    }
+    QMessageBox::critical(
+                this, trUtf8("vmchooser"),
+                trUtf8("Vmchooser failed to run the selected session!"));
+    QApplication::instance()->quit();
 }
 
 void Dialog::on_treeView_expanded(const QModelIndex& index) {
diff --git a/src/session.h b/src/session.h
index 01f8ad5..c3f758c 100644
--- a/src/session.h
+++ b/src/session.h
@@ -39,7 +39,8 @@ class Session {
     virtual QString shortDescription() const = 0;
     virtual QString description() const = 0;
     virtual QIcon icon() const = 0;
-    virtual bool run() const = 0;
+    virtual bool prepareRun() const = 0;
+    virtual void run() const = 0;
     virtual QString execCommand() const { return QString(); }
     virtual QString uuid() const { return QString(); }
     virtual int type() const = 0;
diff --git a/src/userconfig.cpp b/src/userconfig.cpp
index 7bc83fe..31ab9e9 100644
--- a/src/userconfig.cpp
+++ b/src/userconfig.cpp
@@ -90,3 +90,10 @@ bool UserConfig::isNewsHelpOpen()
     init();
     return settings->value(KEY_NEWS_OPEN).toBool();
 }
+
+void UserConfig::sync()
+{
+    if (settings != nullptr) {
+        settings->sync();
+    }
+}
diff --git a/src/userconfig.h b/src/userconfig.h
index 213c4c2..4fdf638 100644
--- a/src/userconfig.h
+++ b/src/userconfig.h
@@ -14,6 +14,7 @@ public:
 	 static void setLastTab(int tab);
 	 static void setLastNewsTime(uint t);
 	 static void setNewsHelpOpen(bool b);
+     static void sync();
 
 private:
 	 UserConfig() {}
diff --git a/src/vsession.cpp b/src/vsession.cpp
index 79ef929..b61c759 100644
--- a/src/vsession.cpp
+++ b/src/vsession.cpp
@@ -21,8 +21,6 @@
 #include "userldapdata.h"
 #include "virtualizer.h"
 
-static QProcess _process;
-
 static const QString VMWARE("vmware");
 static const QString VIRTUALBOX("virtualbox");
 
@@ -65,6 +63,8 @@ static const QStringList directOsNames(
 
 static const QRegularExpression cleanNameRegex("[^a-z0-9]");
 
+QString tmpFileName;
+
 bool VSession::init(const QDomElement& xml) {
     QDomElement settingsNode = this->doc_.createElement("settings");
     this->doc_.appendChild(settingsNode);
@@ -238,44 +238,48 @@ int VSession::priority() const {
     return prio;
 }
 
-bool VSession::run() const {
-    if (_process.state() != QProcess::NotRunning) {
-        qDebug() << "Cannot start vsession while old one is still running";
+bool VSession::prepareRun() const {
+    if (!canRun(true))
         return false;
-    }
-    if (g_debugMode) {
-        qDebug() << "Sarting session " << shortDescription() << " ...";
-    }
-
-    if (g_noVtx && needsVtx()) {
-        QMessageBox::warning(nullptr, QObject::trUtf8("Warning"),
-                QObject::trUtf8("The selected session is based on a 64 bit operating system,"
-                        " but this computer doesn't seem to support this (VT-x/AMD-V not"
-                        " supported by CPU, or disabled in BIOS). You will probably get an"
-                        " error message while the virtualizer is initializing."));
-    }
-
-    QString command = getAttribute(QStringLiteral("command"));
-    if (!command.isEmpty()) {
-        return QProcess::startDetached(command);
-    }
-
     // write xml to temporary file
     QTemporaryFile tmpfile(QDir::tempPath() + QStringLiteral("/vmchooser-XXXXXX.xml"));
     if (!tmpfile.open() ||
         tmpfile.write(this->toXml().toUtf8()) == -1) {
         qDebug() << "Error writing xml to file" << tmpfile.fileName();
+        QMessageBox::critical(
+                nullptr, QObject::trUtf8("vmchooser"),
+                QObject::trUtf8("Error writing temporary XML file for run-virt"));
         return false;
     }
-    // Docs say we should call fileName() before closing to prevent deletion
-    QString tmpFileName = tmpfile.fileName();
+    if (!tmpFileName.isEmpty()) {
+        QFile(tmpFileName).remove();
+    }
+    // Docs say we should call fileName() before closing, to prevent deletion
+    tmpFileName = tmpfile.fileName();
     tmpfile.setAutoRemove(false);
     tmpfile.close();
+    if (g_noVtx && needsVtx()) {
+        QMessageBox::StandardButton ret = QMessageBox::question(nullptr, QObject::tr("Warning"),
+            QObject::tr("The selected session is based on a 64 bit operating system,"
+             " but this computer doesn't seem to support this (VT-x/AMD-V not"
+             " supported by CPU, or disabled in BIOS). You will probably get an"
+             " error message while the virtualizer is initializing.\n\n"
+             "Do you still want to try to start this VM?"),
+            QMessageBox::Yes | QMessageBox::No, QMessageBox::No);
+        if (ret != QMessageBox::Yes)
+            return false;
+    }
+    return true;
+}
 
-    QObject::connect(&_process, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished), QApplication::instance(), &QCoreApplication::quit);
-    _process.start(Config::get(Config::RUNSCRIPT), QStringList(tmpFileName));
-    _process.waitForStarted(10);
-    return _process.state() == QProcess::Starting || _process.state() == QProcess::Running;
+void VSession::run() const {
+    if (g_debugMode) {
+        qDebug() << "Sarting session " << shortDescription() << " ...";
+    }
+    QByteArray fn = tmpFileName.toUtf8();
+    QByteArray script = Config::get(Config::RUNSCRIPT).toUtf8();
+    char *argv[3] = { script.data(), fn.data(), nullptr };
+    execv(script.data(), argv);
 }
 
 int VSession::type() const {
diff --git a/src/vsession.h b/src/vsession.h
index 69ecf13..e571805 100644
--- a/src/vsession.h
+++ b/src/vsession.h
@@ -77,7 +77,9 @@ class VSession : public Session {
 
     QString toXml() const;
 
-    bool run() const;
+    bool prepareRun() const;
+
+    void run() const;
 
     int type() const;
 
diff --git a/src/windowmanager.cpp b/src/windowmanager.cpp
index 2505c89..c05e234 100644
--- a/src/windowmanager.cpp
+++ b/src/windowmanager.cpp
@@ -38,12 +38,16 @@ void ensureRunning()
     });
 }
 
-void stopOwnInstance()
+void stopOwnInstance(bool waitSync)
 {
     if (wm.state() == QProcess::Running) {
         qDebug() << "- Politely terminating spawned WM";
-        wm.terminate();
-        QTimer::singleShot(500, killInstance);
+        if (waitSync) {
+            killInstance();
+        } else {
+            wm.terminate();
+            QTimer::singleShot(500, killInstance);
+        }
     }
 }
 
@@ -51,7 +55,9 @@ void killInstance()
 {
     if (wm.state() == QProcess::Running) {
         wm.terminate();
-        QThread::msleep(50);
+    }
+    for (int i = 0; i < 50 && wm.state() == QProcess::Running; ++i) {
+        QThread::msleep(10);
     }
     if (wm.state() == QProcess::Running) {
         qDebug() << "- Forcefully killing spawned WM";
diff --git a/src/windowmanager.h b/src/windowmanager.h
index c89f983..26d5606 100644
--- a/src/windowmanager.h
+++ b/src/windowmanager.h
@@ -15,7 +15,7 @@ void ensureRunning();
  * terminate it, otherwise leave the current
  * one running.
  */
-void stopOwnInstance();
+void stopOwnInstance(bool waitSync);
 
 }
 
diff --git a/src/xsession.cpp b/src/xsession.cpp
index 05f4d38..5db3b11 100644
--- a/src/xsession.cpp
+++ b/src/xsession.cpp
@@ -2,8 +2,8 @@
 #include <QSettings>
 #include <QLocale>
 #include <QApplication>
-#include <QProcess>
 #include <QStandardPaths>
+#include "unistd.h"
 
 #include "xsession.h"
 #include "globals.h"
@@ -18,10 +18,11 @@ struct DisplayConfig {
 
 static QList< DisplayConfig > priorityList;
 
-void XSession::init(const QString& name, const QString& exec,
+void XSession::init(const QString& name, const QString& exec, const QString& tryExec,
                     const QString& comment, const QString& icon) {
     this->name_ = name;
     this->exec_ = exec;
+    this->tryExec_ = tryExec;
     this->comment_ = comment;
     this->icon_ = icon;
     this->priority_ = 0;
@@ -43,6 +44,7 @@ bool XSession::init(const QString& filename) {
         return false;
     }
     QString exec(settings.value("Exec").toString());
+    QString tryExec(settings.value("TryExec").toString());
     QString locale(QLocale::system().name());
     QString language(locale.split("_").at(0));
     QString defaultName("");
@@ -100,11 +102,10 @@ bool XSession::init(const QString& filename) {
 
     this->name_ = name;
     this->exec_ = exec;
+    this->tryExec_ = tryExec;
     this->comment_ = comment;
     this->icon_ = icon;
 
-    _process = new QProcess();
-
     return true;
 }
 
@@ -113,7 +114,12 @@ bool XSession::isActive() const {
 }
 
 QString XSession::checkCanRunInternal() const {
-    QString exe = this->exec_.left(this->exec_.indexOf(' ')); // -1 means entire string
+    QString exe;
+    if (this->tryExec_.isEmpty()) {
+        exe = this->exec_.left(this->exec_.indexOf(' ')); // -1 means entire string
+    } else {
+        exe = this->tryExec_;
+    }
     QFileInfo fi(exe);
     if (!fi.isAbsolute()) {
         if (!QStandardPaths::findExecutable(exe).isEmpty())
@@ -156,10 +162,21 @@ QIcon XSession::icon() const {
     return retIcon;
 }
 
-bool XSession::run() const {
-    _process->start(this->exec_);
-    QObject::connect(_process, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished), QApplication::instance(), &QCoreApplication::quit);
-    return _process->state() == QProcess::Starting || _process->state() == QProcess::Running;
+bool XSession::prepareRun() const {
+    return canRun(true);
+}
+
+void XSession::run() const {
+    QString command;
+    QString firstToken = this->exec_.left(this->exec_.indexOf(' '));
+    if (!firstToken.contains('=')) { // First token doesn't appear to be environment modification, try to prefix with exec
+        command += "exec ";
+    }
+    command += exec_;
+    qDebug() << "Running via /bin/sh:" << command;
+    QByteArray cmdbin = command.toUtf8();
+    char *argv[4] = { "/bin/sh", "-c", cmdbin.data(), nullptr };
+    execv("/bin/sh", argv);
 }
 
 int XSession::type() const {
diff --git a/src/xsession.h b/src/xsession.h
index e014e9e..7b9135b 100644
--- a/src/xsession.h
+++ b/src/xsession.h
@@ -11,7 +11,7 @@
 
 class XSession : public Session {
   public:
-    void init(const QString& name, const QString& exec,
+    void init(const QString& name, const QString& exec, const QString& tryExec,
               const QString& comment, const QString& icon);
     bool init(const QString& filename);
 
@@ -40,7 +40,9 @@ class XSession : public Session {
 
     QIcon icon() const;
 
-    bool run() const;
+    bool prepareRun() const;
+
+    void run() const;
 
     int type() const;
 
@@ -56,12 +58,11 @@ protected:
   private:
     QString name_;
     QString exec_;
+    QString tryExec_;
     QString comment_;
     QString icon_;
     int priority_;
 
-    QProcess *_process;
-
     static void loadXSessionsConfig();
 
     static inline int find(QChar c, int start, const QString& line)