Get rid of Utils::FileSystemWatcher from SshConnection

When we are starting master process for ssh (-M option specified)
we don't know whether everything went fine, since the process
just starts and doesn't print anything to the output in case
all went fine. It may also happen that when there are issues
the master process finishes soon after it was started.
When everything went fine the master process should create
a socket (specified by ControlPath option). But when we
receive a started() signal from the process, the socket
isn't yet ready. So, in order to detect that connection was
established properly, the old implementation created a
FileSystemWatcher on the expected socket file to appear.

There are 2 issues with the approach above:
1. There might be a race condition inside the started()
   handler of the master process. After checking that
   the expected socket isn't created yet and before setting
   up the file system watcher, the socket file could have
   been created in meantime what wouldn't be noticed.
2. The use of Utils::FileSystemWatcher excludes the usage
   in non-main threads. Thus in general: usage of SshConnection
   outside of main thread is dangerous.

This patch implements it in a different way. Instead of
installing a file system watcher we make use of local command
of ssh master process. We enable it by "PermitLocalCommand=yes"
and specify a local command by "LocalCommand=echo". This means
that local command will be executed after successfully connecting
to the server. Our command is very simple - just empty echo,
which means that we should expect the "\n" on master process
output after successful run. So, instead of connecting to
started() signal we are connected to readyReadStandardOutput()
and detect successful connection after receiving newline char.

This eliminates both issues with the old approach and makes a
step towards thread safe shared ssh connections.

Change-Id: I2e20c82aeff09b297e3cad5644d4d2c956db82d0
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: hjk <hjk@qt.io>
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>
This commit is contained in:
Jarek Kobus
2021-12-14 13:28:55 +01:00
parent f72ea01c65
commit 464e12f5e1

View File

@@ -32,15 +32,12 @@
#include "sshremoteprocess.h" #include "sshremoteprocess.h"
#include "sshsettings.h" #include "sshsettings.h"
#include <utils/filesystemwatcher.h>
#include <utils/fileutils.h> #include <utils/fileutils.h>
#include <utils/hostosinfo.h> #include <utils/hostosinfo.h>
#include <utils/qtcassert.h> #include <utils/qtcassert.h>
#include <utils/qtcprocess.h> #include <utils/qtcprocess.h>
#include <QByteArrayList> #include <QByteArrayList>
#include <QDir>
#include <QFileInfo>
#include <QTemporaryDir> #include <QTemporaryDir>
#include <QTimer> #include <QTimer>
@@ -168,32 +165,10 @@ SshConnection::SshConnection(const SshConnectionParameters &serverInfo, QObject
qRegisterMetaType<QSsh::SftpFileInfo>("QSsh::SftpFileInfo"); qRegisterMetaType<QSsh::SftpFileInfo>("QSsh::SftpFileInfo");
qRegisterMetaType<QList <QSsh::SftpFileInfo> >("QList<QSsh::SftpFileInfo>"); qRegisterMetaType<QList <QSsh::SftpFileInfo> >("QList<QSsh::SftpFileInfo>");
d->connParams = serverInfo; d->connParams = serverInfo;
connect(&d->masterProcess, &QtcProcess::started, [this] { connect(&d->masterProcess, &QtcProcess::readyReadStandardOutput, [this] {
QFileInfo socketInfo(d->socketFilePath()); const QByteArray reply = d->masterProcess.readAllStandardOutput();
if (socketInfo.exists()) { if (reply == "\n")
emitConnected(); emitConnected();
return;
}
auto * const socketWatcher = new FileSystemWatcher(this);
auto * const socketWatcherTimer = new QTimer(this);
const auto socketFileChecker = [this, socketWatcher, socketWatcherTimer] {
if (!QFileInfo::exists(d->socketFilePath()))
return;
socketWatcher->disconnect();
socketWatcher->deleteLater();
socketWatcherTimer->disconnect();
socketWatcherTimer->stop();
socketWatcherTimer->deleteLater();
emitConnected();
};
connect(socketWatcher, &FileSystemWatcher::directoryChanged, socketFileChecker);
socketWatcher->addDirectory(socketInfo.path(), FileSystemWatcher::WatchAllChanges);
if (HostOsInfo::isMacHost()) {
// QTBUG-72455
socketWatcherTimer->setInterval(1000);
connect(socketWatcherTimer, &QTimer::timeout, socketFileChecker);
socketWatcherTimer->start();
}
}); });
connect(&d->masterProcess, &QtcProcess::errorOccurred, [this] (QProcess::ProcessError error) { connect(&d->masterProcess, &QtcProcess::errorOccurred, [this] (QProcess::ProcessError error) {
switch (error) { switch (error) {
@@ -370,7 +345,11 @@ void SshConnection::doConnectToHost()
.arg(d->masterSocketDir->errorString())); .arg(d->masterSocketDir->errorString()));
return; return;
} }
QStringList args = QStringList{"-M", "-N", "-o", "ControlPersist=no"} QStringList args = QStringList{"-M", "-N", "-o", "ControlPersist=no",
"-o", "PermitLocalCommand=yes", // Enable local command
"-o", "LocalCommand=echo"} // Local command is executed after successfully
// connecting to the server. "echo" will print "\n"
// on the process output if everything went fine.
<< d->connectionArgs(sshBinary); << d->connectionArgs(sshBinary);
if (!d->connParams.x11DisplayName.isEmpty()) if (!d->connParams.x11DisplayName.isEmpty())
args.prepend("-X"); args.prepend("-X");