From 0d635e5ad5b6ab518836b6f59c19c2eac24d022f Mon Sep 17 00:00:00 2001 From: Martchus Date: Wed, 18 Apr 2018 23:27:45 +0200 Subject: [PATCH] Refactor launcher * Pass program and arguments directly * Prevent failure on white space in executable path * Use own parser for arguments * Make libsyncthing accessible from launcher --- connector/syncthingprocess.cpp | 88 ++++++++++++++++++++++---- connector/syncthingprocess.h | 8 ++- connector/tests/misctests.cpp | 16 +++++ widgets/misc/syncthinglauncher.cpp | 56 ++++++++++++++-- widgets/misc/syncthinglauncher.h | 17 ++++- widgets/settings/launcheroptionpage.ui | 35 +++++++++- widgets/settings/settings.cpp | 22 ++----- widgets/settings/settings.h | 3 +- widgets/settings/settingsdialog.cpp | 20 ++++-- 9 files changed, 216 insertions(+), 49 deletions(-) diff --git a/connector/syncthingprocess.cpp b/connector/syncthingprocess.cpp index bfe76e7..7def07f 100644 --- a/connector/syncthingprocess.cpp +++ b/connector/syncthingprocess.cpp @@ -21,30 +21,91 @@ SyncthingProcess::SyncthingProcess(QObject *parent) connect(&m_killTimer, &QTimer::timeout, this, &SyncthingProcess::confirmKill); } -void SyncthingProcess::restartSyncthing(const QString &cmd) +QStringList SyncthingProcess::splitArguments(const QString &arguments) +{ + enum { Any, Quote, Slash, Space } lastInput = Any; + bool inQuotes = false; + QStringList result; + QString currentArg; + for (const auto c : arguments) { + switch (c.unicode()) { + case '\"': + case '\'': + switch (lastInput) { + case Slash: + currentArg += c; + lastInput = Any; + break; + default: + inQuotes = !inQuotes; + lastInput = Quote; + } + break; + case '\\': + switch (lastInput) { + case Slash: + currentArg += c; + lastInput = Any; + break; + default: + lastInput = Slash; + } + break; + case ' ': + switch (lastInput) { + case Slash: + currentArg += c; + lastInput = Any; + break; + case Space: + if (inQuotes) { + currentArg += c; + lastInput = Any; + } + break; + default: + if (inQuotes) { + currentArg += c; + lastInput = Any; + } else { + result << currentArg; + currentArg.clear(); + lastInput = Space; + } + } + break; + default: + currentArg += c; + lastInput = Any; + } + } + if (!currentArg.isEmpty()) { + result << currentArg; + } + return result; +} + +void SyncthingProcess::restartSyncthing(const QString &program, const QStringList &arguments) { if (!isRunning()) { - startSyncthing(cmd); + startSyncthing(program, arguments); return; } - m_cmd = cmd; + m_program = program; + m_arguments = arguments; m_manuallyStopped = true; m_killTimer.start(); terminate(); } -void SyncthingProcess::startSyncthing(const QString &cmd) +void SyncthingProcess::startSyncthing(const QString &program, const QStringList &arguments) { if (isRunning()) { return; } m_manuallyStopped = false; m_killTimer.stop(); - if (cmd.isEmpty()) { - start(QProcess::ReadOnly); - } else { - start(cmd, QProcess::ReadOnly); - } + start(program, arguments, QProcess::ReadOnly); } void SyncthingProcess::stopSyncthing() @@ -78,15 +139,16 @@ void SyncthingProcess::handleFinished(int exitCode, QProcess::ExitStatus exitSta Q_UNUSED(exitStatus) m_activeSince = DateTime(); m_killTimer.stop(); - if (!m_cmd.isEmpty()) { - startSyncthing(m_cmd); - m_cmd.clear(); + if (!m_program.isEmpty()) { + startSyncthing(m_program, m_arguments); + m_program.clear(); + m_arguments.clear(); } } void SyncthingProcess::killToRestart() { - if (!m_cmd.isEmpty()) { + if (!m_program.isEmpty()) { kill(); } } diff --git a/connector/syncthingprocess.h b/connector/syncthingprocess.h index a0c7813..659a554 100644 --- a/connector/syncthingprocess.h +++ b/connector/syncthingprocess.h @@ -24,13 +24,14 @@ public: bool isManuallyStopped() const; static SyncthingProcess *mainInstance(); static void setMainInstance(SyncthingProcess *mainInstance); + static QStringList splitArguments(const QString &arguments); Q_SIGNALS: void confirmKill(); public Q_SLOTS: - void restartSyncthing(const QString &cmd); - void startSyncthing(const QString &cmd); + void restartSyncthing(const QString &program, const QStringList &arguments); + void startSyncthing(const QString &program, const QStringList &arguments); void stopSyncthing(); void killSyncthing(); @@ -40,7 +41,8 @@ private Q_SLOTS: void killToRestart(); private: - QString m_cmd; + QString m_program; + QStringList m_arguments; ChronoUtilities::DateTime m_activeSince; QTimer m_killTimer; bool m_manuallyStopped; diff --git a/connector/tests/misctests.cpp b/connector/tests/misctests.cpp index a7cabf6..855d873 100644 --- a/connector/tests/misctests.cpp +++ b/connector/tests/misctests.cpp @@ -1,6 +1,7 @@ #include "../syncthingconfig.h" #include "../syncthingconnection.h" #include "../syncthingconnectionsettings.h" +#include "../syncthingprocess.h" #include "../syncthingservice.h" #include "../utils.h" @@ -29,6 +30,7 @@ using namespace CPPUNIT_NS; class MiscTests : public TestFixture { CPPUNIT_TEST_SUITE(MiscTests); CPPUNIT_TEST(testParsingConfig); + CPPUNIT_TEST(testSplittingArguments); CPPUNIT_TEST(testUtils); CPPUNIT_TEST(testService); CPPUNIT_TEST(testConnectionSettingsAndLoadingSelfSignedCert); @@ -39,6 +41,7 @@ public: MiscTests(); void testParsingConfig(); + void testSplittingArguments(); void testUtils(); void testService(); void testConnectionSettingsAndLoadingSelfSignedCert(); @@ -94,6 +97,19 @@ void MiscTests::testParsingConfig() CPPUNIT_ASSERT(httpsCert.isEmpty() || QFile::exists(httpsCert)); } +/*! + * \brief Test splitting arguments via SyncthingProcess::splitArguments(). + */ +void MiscTests::testSplittingArguments() +{ + CPPUNIT_ASSERT_EQUAL(QStringList(), SyncthingProcess::splitArguments(QString())); + CPPUNIT_ASSERT_EQUAL(QStringList({ QStringLiteral("-simple") }), SyncthingProcess::splitArguments(QStringLiteral("-simple"))); + CPPUNIT_ASSERT_EQUAL(QStringList({ QStringLiteral("-home"), QStringLiteral("some dir"), QStringLiteral("-no-restart") }), + SyncthingProcess::splitArguments(QStringLiteral("-home \"some dir\" -no-restart"))); + CPPUNIT_ASSERT_EQUAL(QStringList({ QStringLiteral("-home"), QStringLiteral("\"some"), QStringLiteral("dir\""), QStringLiteral("-no-restart") }), + SyncthingProcess::splitArguments(QStringLiteral("-home \\\"some dir\\\" -no-restart"))); +} + /*! * \brief Tests utils. */ diff --git a/widgets/misc/syncthinglauncher.cpp b/widgets/misc/syncthinglauncher.cpp index 499c1c1..8f49f21 100644 --- a/widgets/misc/syncthinglauncher.cpp +++ b/widgets/misc/syncthinglauncher.cpp @@ -15,6 +15,7 @@ SyncthingLauncher *SyncthingLauncher::s_mainInstance = nullptr; SyncthingLauncher::SyncthingLauncher(QObject *parent) : QObject(parent) + , m_useLibSyncthing(false) { connect(&m_process, &SyncthingProcess::readyRead, this, &SyncthingLauncher::handleProcessReadyRead); connect(&m_process, static_cast(&SyncthingProcess::finished), this, @@ -31,22 +32,41 @@ bool SyncthingLauncher::isLibSyncthingAvailable() #endif } -void SyncthingLauncher::launch(const QString &cmd) +/*! + * \brief Launches a Syncthing instance using the specified \a arguments. + * \remarks To use the internal library, leave \a program empty. Otherwise it must be the path the external Syncthing executable. + */ +void SyncthingLauncher::launch(const QString &program, const QStringList &arguments) { if (isRunning()) { return; } m_manuallyStopped = false; - m_process.startSyncthing(cmd); + if (!program.isEmpty()) { + m_process.startSyncthing(program, arguments); + } else { + vector utf8Arguments{ "-no-restart", "-no-browser" }; + utf8Arguments.reserve(utf8Arguments.size() + static_cast(arguments.size())); + for (const auto &arg : arguments) { + const auto utf8Data(arg.toUtf8()); + utf8Arguments.emplace_back(utf8Data.data(), utf8Data.size()); + } + m_future = QtConcurrent::run( + this, static_cast &)>(&SyncthingLauncher::runLibSyncthing), utf8Arguments); + } } +/*! + * \brief Launches a Syncthing instance using the internal library with the specified \a runtimeOptions. + */ void SyncthingLauncher::launch(const LibSyncthing::RuntimeOptions &runtimeOptions) { if (isRunning()) { return; } m_manuallyStopped = false; - m_future = QtConcurrent::run(this, &SyncthingLauncher::runLibSyncthing, runtimeOptions); + m_future = QtConcurrent::run( + this, static_cast(&SyncthingLauncher::runLibSyncthing), runtimeOptions); } void SyncthingLauncher::terminate() @@ -87,13 +107,28 @@ void SyncthingLauncher::handleProcessFinished(int exitCode, QProcess::ExitStatus emit exited(exitCode, exitStatus); } +static const char *const logLevelStrings[] = { + "[DEBUG] ", + "[VERBOSE] ", + "[INFO] ", + "[WARNING] ", + "[FATAL] ", +}; + void SyncthingLauncher::handleLoggingCallback(LibSyncthing::LogLevel level, const char *message, size_t messageSize) { #ifdef SYNCTHING_WIDGETS_USE_LIBSYNCTHING if (level < LibSyncthing::LogLevel::Info) { return; } - emit outputAvailable(QByteArray(message, static_cast(max(numeric_limits::max(), messageSize)))); + QByteArray messageData; + messageSize = min(numeric_limits::max() - 20, messageSize); + messageData.reserve(static_cast(messageSize) + 20); + messageData.append(logLevelStrings[static_cast(level)]); + messageData.append(message, static_cast(messageSize)); + messageData.append('\n'); + + emit outputAvailable(move(messageData)); #else VAR_UNUSED(level) VAR_UNUSED(message) @@ -114,6 +149,19 @@ void SyncthingLauncher::runLibSyncthing(const LibSyncthing::RuntimeOptions &runt #endif } +void SyncthingLauncher::runLibSyncthing(const std::vector &arguments) +{ +#ifdef SYNCTHING_WIDGETS_USE_LIBSYNCTHING + LibSyncthing::setLoggingCallback(bind(&SyncthingLauncher::handleLoggingCallback, this, _1, _2, _3)); + const auto exitCode = LibSyncthing::runSyncthing(arguments); + emit exited(static_cast(exitCode), exitCode == 0 ? QProcess::NormalExit : QProcess::CrashExit); +#else + VAR_UNUSED(arguments) + emit outputAvailable("libsyncthing support not enabled"); + emit exited(-1, QProcess::CrashExit); +#endif +} + SyncthingLauncher &syncthingLauncher() { static SyncthingLauncher launcher; diff --git a/widgets/misc/syncthinglauncher.h b/widgets/misc/syncthinglauncher.h index 27c0340..d2f9c64 100644 --- a/widgets/misc/syncthinglauncher.h +++ b/widgets/misc/syncthinglauncher.h @@ -19,6 +19,7 @@ class SYNCTHINGWIDGETS_EXPORT SyncthingLauncher : public QObject { Q_PROPERTY(bool running READ isRunning NOTIFY runningChanged) Q_PROPERTY(ChronoUtilities::DateTime activeSince READ activeSince) Q_PROPERTY(bool manuallyStopped READ isManuallyStopped) + Q_PROPERTY(bool useLibSyncthing READ isUseLibSyncthing WRITE setUseLibSyncthing) public: explicit SyncthingLauncher(QObject *parent = nullptr); @@ -27,6 +28,7 @@ public: ChronoUtilities::DateTime activeSince() const; bool isActiveFor(unsigned int atLeastSeconds) const; bool isManuallyStopped() const; + bool isUseLibSyncthing() const; static bool isLibSyncthingAvailable(); static SyncthingLauncher *mainInstance(); static void setMainInstance(SyncthingLauncher *mainInstance); @@ -38,7 +40,8 @@ Q_SIGNALS: void exited(int exitCode, QProcess::ExitStatus exitStatus); public Q_SLOTS: - void launch(const QString &cmd); + void setUseLibSyncthing(bool useLibSyncthing); + void launch(const QString &program, const QStringList &arguments); void launch(const LibSyncthing::RuntimeOptions &runtimeOptions); void terminate(); void kill(); @@ -48,12 +51,14 @@ private Q_SLOTS: void handleProcessFinished(int exitCode, QProcess::ExitStatus exitStatus); void handleLoggingCallback(LibSyncthing::LogLevel, const char *message, std::size_t messageSize); void runLibSyncthing(const LibSyncthing::RuntimeOptions &runtimeOptions); + void runLibSyncthing(const std::vector &arguments); private: SyncthingProcess m_process; QFuture m_future; ChronoUtilities::DateTime m_futureStarted; bool m_manuallyStopped; + bool m_useLibSyncthing; static SyncthingLauncher *s_mainInstance; }; @@ -83,6 +88,16 @@ inline bool SyncthingLauncher::isManuallyStopped() const return m_manuallyStopped; } +inline bool SyncthingLauncher::isUseLibSyncthing() const +{ + return m_useLibSyncthing; +} + +inline void SyncthingLauncher::setUseLibSyncthing(bool useLibSyncthing) +{ + m_useLibSyncthing = useLibSyncthing; +} + inline SyncthingLauncher *SyncthingLauncher::mainInstance() { return s_mainInstance; diff --git a/widgets/settings/launcheroptionpage.ui b/widgets/settings/launcheroptionpage.ui index f666b22..05da7ff 100644 --- a/widgets/settings/launcheroptionpage.ui +++ b/widgets/settings/launcheroptionpage.ui @@ -2,6 +2,14 @@ QtGui::LauncherOptionPage + + + 0 + 0 + 507 + 391 + + Syncthing launcher @@ -47,16 +55,23 @@ - + Arguments - + + + + + Use built-in Syncthing library (experimental) + + + @@ -215,5 +230,21 @@ + + useBuiltInVersionCheckBox + toggled(bool) + syncthingPathSelection + setDisabled(bool) + + + 320 + 58 + + + 320 + 36 + + + diff --git a/widgets/settings/settings.cpp b/widgets/settings/settings.cpp index 494efa1..5a1f70d 100644 --- a/widgets/settings/settings.cpp +++ b/widgets/settings/settings.cpp @@ -52,20 +52,6 @@ namespace Settings { */ static unordered_map toolProcesses; -QString Launcher::syncthingCmd() const -{ - return syncthingPath % QChar(' ') % syncthingArgs; -} - -QString Launcher::toolCmd(const QString &tool) const -{ - const ToolParameter toolParams = tools.value(tool); - if (toolParams.path.isEmpty()) { - return QString(); - } - return toolParams.path % QChar(' ') % toolParams.args; -} - SyncthingProcess &Launcher::toolProcess(const QString &tool) { return toolProcesses[tool]; @@ -92,12 +78,12 @@ void Launcher::autostart() const auto *const launcher(SyncthingLauncher::mainInstance()); // TODO: allow using libsyncthing if (enabled && !syncthingPath.isEmpty() && launcher) { - launcher->launch(syncthingCmd()); + launcher->launch(useLibSyncthing ? QString() : syncthingPath, SyncthingProcess::splitArguments(syncthingArgs)); } for (auto i = tools.cbegin(), end = tools.cend(); i != end; ++i) { const ToolParameter &toolParams = i.value(); if (toolParams.autostart && !toolParams.path.isEmpty()) { - toolProcesses[i.key()].startSyncthing(toolParams.path % QChar(' ') % toolParams.args); + toolProcesses[i.key()].startSyncthing(toolParams.path, SyncthingProcess::splitArguments(toolParams.args)); } } } @@ -194,6 +180,7 @@ void restore() settings.beginGroup(QStringLiteral("startup")); auto &launcher = v.launcher; launcher.enabled = settings.value(QStringLiteral("syncthingAutostart"), launcher.enabled).toBool(); + launcher.useLibSyncthing = settings.value(QStringLiteral("useLibSyncthing"), launcher.useLibSyncthing).toBool(); launcher.syncthingPath = settings.value(QStringLiteral("syncthingPath"), launcher.syncthingPath).toString(); launcher.syncthingArgs = settings.value(QStringLiteral("syncthingArgs"), launcher.syncthingArgs).toString(); launcher.considerForReconnect = settings.value(QStringLiteral("considerLauncherForReconnect"), launcher.considerForReconnect).toBool(); @@ -206,8 +193,6 @@ void restore() toolParams.args = settings.value(QStringLiteral("args"), toolParams.args).toString(); settings.endGroup(); } - for (auto i = launcher.tools.cbegin(), end = launcher.tools.cend(); i != end; ++i) { - } settings.endGroup(); #ifdef LIB_SYNCTHING_CONNECTOR_SUPPORT_SYSTEMD auto &systemd = v.systemd; @@ -280,6 +265,7 @@ void save() settings.beginGroup(QStringLiteral("startup")); const auto &launcher = v.launcher; settings.setValue(QStringLiteral("syncthingAutostart"), launcher.enabled); + settings.setValue(QStringLiteral("useLibSyncthing"), launcher.useLibSyncthing); settings.setValue(QStringLiteral("syncthingPath"), launcher.syncthingPath); settings.setValue(QStringLiteral("syncthingArgs"), launcher.syncthingArgs); settings.setValue(QStringLiteral("considerLauncherForReconnect"), launcher.considerForReconnect); diff --git a/widgets/settings/settings.h b/widgets/settings/settings.h index c9bc218..7746203 100644 --- a/widgets/settings/settings.h +++ b/widgets/settings/settings.h @@ -59,6 +59,7 @@ struct SYNCTHINGWIDGETS_EXPORT ToolParameter { struct SYNCTHINGWIDGETS_EXPORT Launcher { bool enabled = false; + bool useLibSyncthing = false; QString syncthingPath = #ifdef PLATFORM_WINDOWS QStringLiteral("syncthing.exe"); @@ -68,8 +69,6 @@ struct SYNCTHINGWIDGETS_EXPORT Launcher { QString syncthingArgs; QHash tools; bool considerForReconnect = false; - QString syncthingCmd() const; - QString toolCmd(const QString &tool) const; static Data::SyncthingProcess &toolProcess(const QString &tool); static std::vector allProcesses(); void autostart() const; diff --git a/widgets/settings/settingsdialog.cpp b/widgets/settings/settingsdialog.cpp index c704c24..6ab4d8f 100644 --- a/widgets/settings/settingsdialog.cpp +++ b/widgets/settings/settingsdialog.cpp @@ -648,6 +648,7 @@ QWidget *LauncherOptionPage::setupWidget() const auto running(isRunning()); ui()->launchNowPushButton->setHidden(running); ui()->stopPushButton->setHidden(!running); + ui()->useBuiltInVersionCheckBox->setHidden(!SyncthingLauncher::isLibSyncthingAvailable()); // connect signals & slots if (m_process) { m_connections << QObject::connect(m_process, &SyncthingProcess::readyRead, bind(&LauncherOptionPage::handleSyncthingReadyRead, this)); @@ -655,8 +656,8 @@ QWidget *LauncherOptionPage::setupWidget() static_cast(&SyncthingProcess::finished), bind(&LauncherOptionPage::handleSyncthingExited, this, _1, _2)); } else if (m_launcher) { - m_connections << QObject::connect( - m_launcher, &SyncthingLauncher::outputAvailable, bind(&LauncherOptionPage::handleSyncthingOutputAvailable, this, _1)); + m_connections << QObject::connect(m_launcher, &SyncthingLauncher::outputAvailable, ui()->logTextEdit, + bind(&LauncherOptionPage::handleSyncthingOutputAvailable, this, _1), Qt::QueuedConnection); m_connections << QObject::connect(m_launcher, &SyncthingLauncher::exited, bind(&LauncherOptionPage::handleSyncthingExited, this, _1, _2)); } QObject::connect(ui()->launchNowPushButton, &QPushButton::clicked, bind(&LauncherOptionPage::launch, this)); @@ -669,6 +670,7 @@ bool LauncherOptionPage::apply() auto &settings = values().launcher; if (m_tool.isEmpty()) { settings.enabled = ui()->enabledCheckBox->isChecked(); + settings.useLibSyncthing = ui()->useBuiltInVersionCheckBox->isChecked(); settings.syncthingPath = ui()->syncthingPathSelection->lineEdit()->text(); settings.syncthingArgs = ui()->argumentsLineEdit->text(); settings.considerForReconnect = ui()->considerForReconnectCheckBox->isChecked(); @@ -686,11 +688,15 @@ void LauncherOptionPage::reset() const auto &settings = values().launcher; if (m_tool.isEmpty()) { ui()->enabledCheckBox->setChecked(settings.enabled); + ui()->useBuiltInVersionCheckBox->setChecked(settings.useLibSyncthing); + ui()->useBuiltInVersionCheckBox->setVisible(settings.useLibSyncthing || SyncthingLauncher::isLibSyncthingAvailable()); ui()->syncthingPathSelection->lineEdit()->setText(settings.syncthingPath); ui()->argumentsLineEdit->setText(settings.syncthingArgs); ui()->considerForReconnectCheckBox->setChecked(settings.considerForReconnect); } else { const ToolParameter params = settings.tools.value(m_tool); + ui()->useBuiltInVersionCheckBox->setChecked(false); + ui()->useBuiltInVersionCheckBox->setVisible(false); ui()->enabledCheckBox->setChecked(params.autostart); ui()->syncthingPathSelection->lineEdit()->setText(params.path); ui()->argumentsLineEdit->setText(params.args); @@ -709,7 +715,7 @@ void LauncherOptionPage::handleSyncthingOutputAvailable(const QByteArray &output } QTextCursor cursor(ui()->logTextEdit->textCursor()); cursor.movePosition(QTextCursor::End); - cursor.insertText(QString::fromLocal8Bit(output)); + cursor.insertText(QString::fromUtf8(output)); if (ui()->ensureCursorVisibleCheckBox->isChecked()) { ui()->logTextEdit->ensureCursorVisible(); } @@ -754,11 +760,13 @@ void LauncherOptionPage::launch() ui()->stopPushButton->show(); ui()->stopPushButton->setText(QCoreApplication::translate("QtGui::LauncherOptionPage", "Stop launched instance")); m_kill = false; + const auto launcherSettings(values().launcher); if (m_tool.isEmpty()) { - // TODO: allow using libsyncthing - m_launcher->launch(values().launcher.syncthingCmd()); + m_launcher->launch(launcherSettings.useLibSyncthing ? QString() : launcherSettings.syncthingPath, + SyncthingProcess::splitArguments(launcherSettings.syncthingArgs)); } else { - m_process->startSyncthing(values().launcher.toolCmd(m_tool)); + const auto toolParams(launcherSettings.tools.value(m_tool)); + m_process->startSyncthing(toolParams.path, SyncthingProcess::splitArguments(toolParams.args)); } }