From 5b80c6045c5306e5e61f963d30c0a9b8773e5407 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrius=20=C5=A0tikonas?= Date: Thu, 10 Sep 2020 23:54:23 +0100 Subject: [PATCH 1/5] Fix fstab editing functionality. In the old code QByteArray fstabContents was actually empty. Also, writeData function was opening file in append mode, thus nothing was actually written. Split writeData function into two: * one for block devices * another for writing fstab file BUG: 417205 --- src/core/fstab.cpp | 4 +-- src/util/externalcommand.cpp | 41 +++++++++++++++++++++++++++++- src/util/externalcommand.h | 1 + src/util/externalcommandhelper.cpp | 38 ++++++++++++++++++++++++--- src/util/externalcommandhelper.h | 2 ++ 5 files changed, 80 insertions(+), 6 deletions(-) diff --git a/src/core/fstab.cpp b/src/core/fstab.cpp index a91686f..630cd0c 100644 --- a/src/core/fstab.cpp +++ b/src/core/fstab.cpp @@ -260,12 +260,12 @@ static void writeEntry(QTextStream& s, const FstabEntry& entry) bool writeMountpoints(const FstabEntryList& fstabEntries, const QString& filename) { Report report(nullptr); - QByteArray fstabContents; + QString fstabContents; QTextStream out(&fstabContents); for (const auto &e : fstabEntries) writeEntry(out, e); ExternalCommand cmd; - return cmd.writeData(report, fstabContents, filename, 0); + return cmd.createFile(report, fstabContents.toLocal8Bit(), filename); } diff --git a/src/util/externalcommand.cpp b/src/util/externalcommand.cpp index 9ba2fac..49f78e1 100644 --- a/src/util/externalcommand.cpp +++ b/src/util/externalcommand.cpp @@ -232,7 +232,7 @@ bool ExternalCommand::writeData(Report& commandReport, const QByteArray& buffer, auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days - + QDBusPendingCall pcall = interface->writeData(buffer, deviceNode, firstByte); QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); @@ -255,6 +255,45 @@ bool ExternalCommand::writeData(Report& commandReport, const QByteArray& buffer, return rval; } +bool ExternalCommand::createFile(Report& commandReport, const QByteArray& buffer, const QString& deviceNode) +{ + d->m_Report = commandReport.newChild(); + if (report()) + report()->setCommand(xi18nc("@info:status", "Command: %1 %2", command(), args().join(QStringLiteral(" ")))); + + bool rval = true; + + if (!QDBusConnection::systemBus().isConnected()) { + qWarning() << QDBusConnection::systemBus().lastError().message(); + return false; + } + + auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), + QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); + interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days + + QDBusPendingCall pcall = interface->createFile(buffer, deviceNode); + + QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); + QEventLoop loop; + + auto exitLoop = [&] (QDBusPendingCallWatcher *watcher) { + loop.exit(); + if (watcher->isError()) + qWarning() << watcher->error(); + else { + QDBusPendingReply reply = *watcher; + rval = reply.argumentAt<0>(); + } + setExitCode(!rval); + }; + + connect(watcher, &QDBusPendingCallWatcher::finished, exitLoop); + loop.exec(); + + return rval; +} + bool ExternalCommand::write(const QByteArray& input) { diff --git a/src/util/externalcommand.h b/src/util/externalcommand.h index be56818..b2152a6 100644 --- a/src/util/externalcommand.h +++ b/src/util/externalcommand.h @@ -71,6 +71,7 @@ public: public: bool copyBlocks(const CopySource& source, CopyTarget& target); bool writeData(Report& commandReport, const QByteArray& buffer, const QString& deviceNode, const quint64 firstByte); // same as copyBlocks but from QByteArray + bool createFile(Report& commandReport, const QByteArray& buffer, const QString& deviceNode); // same as writeData but creates a new file /**< @param cmd the command to run */ void setCommand(const QString& cmd); diff --git a/src/util/externalcommandhelper.cpp b/src/util/externalcommandhelper.cpp index 9e96966..6ccdda0 100644 --- a/src/util/externalcommandhelper.cpp +++ b/src/util/externalcommandhelper.cpp @@ -118,7 +118,7 @@ bool ExternalCommandHelper::readData(const QString& sourceDevice, QByteArray& bu return true; } -/** Writes the data from buffer to a given device or file. +/** Writes the data from buffer to a given device. @param targetDevice device or file to write to @param buffer the data that we write @param offset offset where to begin writing @@ -128,7 +128,8 @@ bool ExternalCommandHelper::writeData(const QString &targetDevice, const QByteAr { QFile device(targetDevice); - if (!device.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Unbuffered)) { + auto flags = QIODevice::WriteOnly | QIODevice::Unbuffered | QIODevice::Append; + if (!device.open(flags)) { qCritical() << xi18n("Could not open device %1 for writing.", targetDevice); return false; } @@ -146,6 +147,29 @@ bool ExternalCommandHelper::writeData(const QString &targetDevice, const QByteAr return true; } +/** Creates a new file with given contents. + @param filePath file to write to + @param fileContents the data that we write + @return true on success +*/ +bool ExternalCommandHelper::createFile(const QString &filePath, const QByteArray& fileContents) +{ + QFile device(filePath); + + auto flags = QIODevice::WriteOnly | QIODevice::Unbuffered; + if (!device.open(flags)) { + qCritical() << xi18n("Could not open file %1 for writing.", filePath); + return false; + } + + if (device.write(fileContents) != fileContents.size()) { + qCritical() << xi18n("Could not write to file %1.", filePath); + return false; + } + + return true; +} + // If targetDevice is empty then return QByteArray with data that was read from disk. QVariantMap ExternalCommandHelper::copyblocks(const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize) { @@ -239,12 +263,20 @@ QVariantMap ExternalCommandHelper::copyblocks(const QString& sourceDevice, const bool ExternalCommandHelper::writeData(const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte) { // Do not allow using this helper for writing to arbitrary location - if ( targetDevice.left(5) != QStringLiteral("/dev/") && !targetDevice.contains(QStringLiteral("/etc/fstab"))) + if ( targetDevice.left(5) != QStringLiteral("/dev/") ) return false; return writeData(targetDevice, buffer, targetFirstByte); } +bool ExternalCommandHelper::createFile(const QByteArray& fileContents, const QString& filePath) +{ + // Do not allow using this helper for writing to arbitrary location + if ( !filePath.contains(QStringLiteral("/etc/fstab")) ) + return false; + + return createFile(filePath, fileContents); +} QVariantMap ExternalCommandHelper::start(const QString& command, const QStringList& arguments, const QByteArray& input, const int processChannelMode) { diff --git a/src/util/externalcommandhelper.h b/src/util/externalcommandhelper.h index 6d7029b..8c1e6e0 100644 --- a/src/util/externalcommandhelper.h +++ b/src/util/externalcommandhelper.h @@ -41,12 +41,14 @@ Q_SIGNALS: public: bool readData(const QString& sourceDevice, QByteArray& buffer, const qint64 offset, const qint64 size); bool writeData(const QString& targetDevice, const QByteArray& buffer, const qint64 offset); + bool createFile(const QString& filePath, const QByteArray& fileContents); public Q_SLOTS: ActionReply init(const QVariantMap& args); Q_SCRIPTABLE QVariantMap start(const QString& command, const QStringList& arguments, const QByteArray& input, const int processChannelMode); Q_SCRIPTABLE QVariantMap copyblocks(const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize); Q_SCRIPTABLE bool writeData(const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte); + Q_SCRIPTABLE bool createFile(const QByteArray& fileContents, const QString& filePath); Q_SCRIPTABLE void exit(); private: From 2ed99536948a7310f3ccf4f8f955fd2829866784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrius=20=C5=A0tikonas?= Date: Fri, 11 Sep 2020 20:27:24 +0100 Subject: [PATCH 2/5] Simplify some External Command functions. --- src/core/fstab.cpp | 3 +- src/util/externalcommand.cpp | 78 +++++++++++------------------------- src/util/externalcommand.h | 6 ++- 3 files changed, 30 insertions(+), 57 deletions(-) diff --git a/src/core/fstab.cpp b/src/core/fstab.cpp index 630cd0c..fac5f54 100644 --- a/src/core/fstab.cpp +++ b/src/core/fstab.cpp @@ -259,7 +259,6 @@ static void writeEntry(QTextStream& s, const FstabEntry& entry) bool writeMountpoints(const FstabEntryList& fstabEntries, const QString& filename) { - Report report(nullptr); QString fstabContents; QTextStream out(&fstabContents); @@ -267,5 +266,5 @@ bool writeMountpoints(const FstabEntryList& fstabEntries, const QString& filenam writeEntry(out, e); ExternalCommand cmd; - return cmd.createFile(report, fstabContents.toLocal8Bit(), filename); + return cmd.createFile(fstabContents.toLocal8Bit(), filename); } diff --git a/src/util/externalcommand.cpp b/src/util/externalcommand.cpp index 49f78e1..972be75 100644 --- a/src/util/externalcommand.cpp +++ b/src/util/externalcommand.cpp @@ -124,11 +124,6 @@ bool ExternalCommand::start(int timeout) if (command().isEmpty()) return false; - if (!QDBusConnection::systemBus().isConnected()) { - qWarning() << QDBusConnection::systemBus().lastError().message(); - return false; - } - if (report()) report()->setCommand(xi18nc("@info:status", "Command: %1 %2", command(), args().join(QStringLiteral(" ")))); @@ -139,10 +134,9 @@ bool ExternalCommand::start(int timeout) if (cmd.isEmpty()) cmd = QStandardPaths::findExecutable(command(), { QStringLiteral("/sbin/"), QStringLiteral("/usr/sbin/"), QStringLiteral("/usr/local/sbin/") }); - auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), - QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); - - interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days + auto interface = helperInterface(); + if (!interface) + return false; bool rval = false; @@ -176,18 +170,13 @@ bool ExternalCommand::copyBlocks(const CopySource& source, CopyTarget& target) bool rval = true; const qint64 blockSize = 10 * 1024 * 1024; // number of bytes per block to copy - if (!QDBusConnection::systemBus().isConnected()) { - qWarning() << QDBusConnection::systemBus().lastError().message(); - return false; - } - // TODO KF6:Use new signal-slot syntax connect(m_job, SIGNAL(percent(KJob*, unsigned long)), this, SLOT(emitProgress(KJob*, unsigned long))); connect(m_job, &KAuth::ExecuteJob::newData, this, &ExternalCommand::emitReport); - auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), - QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); - interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days + auto interface = helperInterface(); + if (!interface) + return false; QDBusPendingCall pcall = interface->copyblocks(source.path(), source.firstByte(), source.length(), target.path(), target.firstByte(), blockSize); @@ -222,58 +211,40 @@ bool ExternalCommand::writeData(Report& commandReport, const QByteArray& buffer, if (report()) report()->setCommand(xi18nc("@info:status", "Command: %1 %2", command(), args().join(QStringLiteral(" ")))); - bool rval = true; - - if (!QDBusConnection::systemBus().isConnected()) { - qWarning() << QDBusConnection::systemBus().lastError().message(); + auto interface = helperInterface(); + if (!interface) return false; - } - - auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), - QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); - interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days QDBusPendingCall pcall = interface->writeData(buffer, deviceNode, firstByte); - - QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); - QEventLoop loop; - - auto exitLoop = [&] (QDBusPendingCallWatcher *watcher) { - loop.exit(); - if (watcher->isError()) - qWarning() << watcher->error(); - else { - QDBusPendingReply reply = *watcher; - rval = reply.argumentAt<0>(); - } - setExitCode(!rval); - }; - - connect(watcher, &QDBusPendingCallWatcher::finished, exitLoop); - loop.exec(); - - return rval; + return waitForDbusReply(pcall); } -bool ExternalCommand::createFile(Report& commandReport, const QByteArray& buffer, const QString& deviceNode) +bool ExternalCommand::createFile(const QByteArray& buffer, const QString& deviceNode) { - d->m_Report = commandReport.newChild(); - if (report()) - report()->setCommand(xi18nc("@info:status", "Command: %1 %2", command(), args().join(QStringLiteral(" ")))); + auto interface = helperInterface(); + if (!interface) + return false; - bool rval = true; + QDBusPendingCall pcall = interface->createFile(buffer, deviceNode); + return waitForDbusReply(pcall); +} +OrgKdeKpmcoreExternalcommandInterface* ExternalCommand::helperInterface() +{ if (!QDBusConnection::systemBus().isConnected()) { qWarning() << QDBusConnection::systemBus().lastError().message(); - return false; + return nullptr; } auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days + return interface; +} - QDBusPendingCall pcall = interface->createFile(buffer, deviceNode); - +bool ExternalCommand::waitForDbusReply(QDBusPendingCall &pcall) +{ + bool rval = true; QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); QEventLoop loop; @@ -294,7 +265,6 @@ bool ExternalCommand::createFile(Report& commandReport, const QByteArray& buffer return rval; } - bool ExternalCommand::write(const QByteArray& input) { if ( qEnvironmentVariableIsSet( "KPMCORE_DEBUG" )) diff --git a/src/util/externalcommand.h b/src/util/externalcommand.h index b2152a6..723f03e 100644 --- a/src/util/externalcommand.h +++ b/src/util/externalcommand.h @@ -38,6 +38,8 @@ class Report; class CopySource; class CopyTarget; class QDBusInterface; +class QDBusPendingCall; +class OrgKdeKpmcoreExternalcommandInterface; struct ExternalCommandPrivate; @@ -71,7 +73,7 @@ public: public: bool copyBlocks(const CopySource& source, CopyTarget& target); bool writeData(Report& commandReport, const QByteArray& buffer, const QString& deviceNode, const quint64 firstByte); // same as copyBlocks but from QByteArray - bool createFile(Report& commandReport, const QByteArray& buffer, const QString& deviceNode); // same as writeData but creates a new file + bool createFile(const QByteArray& buffer, const QString& deviceNode); // similar to writeData but creates a new file /**< @param cmd the command to run */ void setCommand(const QString& cmd); @@ -129,6 +131,8 @@ public Q_SLOTS: private: void setExitCode(int i); void onReadOutput(); + bool waitForDbusReply(QDBusPendingCall &pcall); + OrgKdeKpmcoreExternalcommandInterface* helperInterface(); private: std::unique_ptr d; From 1d195b00da124c998c7736671c88099246f34766 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrius=20=C5=A0tikonas?= Date: Mon, 14 Sep 2020 02:15:20 +0100 Subject: [PATCH 3/5] Prettier formatting of fstab file. --- src/core/fstab.cpp | 54 +++++++++++++++++++++++++++++++--------------- src/core/fstab.h | 5 +++++ 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/core/fstab.cpp b/src/core/fstab.cpp index fac5f54..69abcca 100644 --- a/src/core/fstab.cpp +++ b/src/core/fstab.cpp @@ -21,6 +21,8 @@ #include "util/externalcommand.h" #include "util/report.h" +#include + #if defined(Q_OS_LINUX) #include #endif @@ -34,6 +36,8 @@ static void parseFsSpec(const QString& m_fsSpec, FstabEntry::Type& m_entryType, QString& m_deviceNode); static QString findBlkIdDevice(const char *token, const QString& value); +static void writeEntry(QTextStream& s, const FstabEntry& entry, std::array columnWidth); +std::array fstabColumnWidth(const FstabEntryList& fstabEntries); struct FstabEntryPrivate { @@ -142,6 +146,11 @@ const QStringList& FstabEntry::options() const return d->m_options; } +const QString FstabEntry::optionsString() const +{ + return options().size() > 0 ? options().join(QLatin1Char(',')) : QStringLiteral("defaults"); +} + int FstabEntry::dumpFreq() const { return d->m_dumpFreq; @@ -232,28 +241,37 @@ static void parseFsSpec(const QString& m_fsSpec, FstabEntry::Type& m_entryType, } } -static void writeEntry(QTextStream& s, const FstabEntry& entry) + +// Used to nicely format fstab file +std::array fstabColumnWidth(const FstabEntryList& fstabEntries) +{ + std::array columnWidth; + +#define FIELD_WIDTH(x) 3 + std::max_element(fstabEntries.begin(), fstabEntries.end(), [](const FstabEntry& a, const FstabEntry& b) {return a.x().length() < b.x().length(); })->x().length(); + + columnWidth[0] = FIELD_WIDTH(fsSpec); + columnWidth[1] = FIELD_WIDTH(mountPoint); + columnWidth[2] = FIELD_WIDTH(type); + columnWidth[3] = FIELD_WIDTH(optionsString); + + return columnWidth; +} + +static void writeEntry(QTextStream& s, const FstabEntry& entry, std::array columnWidth) { if (entry.entryType() == FstabEntry::Type::comment) { s << entry.comment() << "\n"; return; } - QString options; - if (entry.options().size() > 0) { - options = entry.options().join(QLatin1Char(',')); - if (options.isEmpty()) - options = QStringLiteral("defaults"); - } - else - options = QStringLiteral("defaults"); - - s << entry.fsSpec() << "\t" - << (entry.mountPoint().isEmpty() ? QStringLiteral("none") : entry.mountPoint()) << "\t" - << entry.type() << "\t" - << options << "\t" - << entry.dumpFreq() << "\t" - << entry.passNumber() << "\t" + s.setFieldAlignment(QTextStream::AlignLeft); + s.setFieldWidth(columnWidth[0]); + s << entry.fsSpec() + << qSetFieldWidth(columnWidth[1]) << (entry.mountPoint().isEmpty() ? QStringLiteral("none") : entry.mountPoint()) + << qSetFieldWidth(columnWidth[2]) << entry.type() + << qSetFieldWidth(columnWidth[3]) << entry.optionsString() << qSetFieldWidth(0) + << entry.dumpFreq() << " " + << entry.passNumber() << " " << entry.comment() << "\n"; } @@ -262,8 +280,10 @@ bool writeMountpoints(const FstabEntryList& fstabEntries, const QString& filenam QString fstabContents; QTextStream out(&fstabContents); + std::array columnWidth = fstabColumnWidth(fstabEntries); + for (const auto &e : fstabEntries) - writeEntry(out, e); + writeEntry(out, e, columnWidth); ExternalCommand cmd; return cmd.createFile(fstabContents.toLocal8Bit(), filename); diff --git a/src/core/fstab.h b/src/core/fstab.h index d158813..cdc7e47 100644 --- a/src/core/fstab.h +++ b/src/core/fstab.h @@ -66,6 +66,11 @@ public: */ const QStringList& options() const; + /** + * @return the mount options associated with the file system + */ + const QString optionsString() const; + /** * @return the fs_freq field of fstab entry */ From a928c62a7d0746c6bf88ed11c7aab394b057b15f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrius=20=C5=A0tikonas?= Date: Mon, 14 Sep 2020 02:19:14 +0100 Subject: [PATCH 4/5] Make fstab parsing slightly more readable. --- src/core/fstab.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/core/fstab.cpp b/src/core/fstab.cpp index 69abcca..e1b502a 100644 --- a/src/core/fstab.cpp +++ b/src/core/fstab.cpp @@ -92,15 +92,20 @@ FstabEntryList readFstabEntries( const QString& fstabPath ) // (4) dump frequency (optional, defaults to 0), no comment is allowed if omitted, // (5) pass number (optional, defaults to 0), no comment is allowed if omitted, // (#) comment (optional). + auto fsSpec = splitLine.at(0); + auto mountPoint = splitLine.at(1); + auto fsType = splitLine.at(2); + auto options = splitLine.at(3); + switch (splitLine.length()) { case 4: - fstabEntries.push_back( {splitLine.at(0), splitLine.at(1), splitLine.at(2), splitLine.at(3) } ); + fstabEntries.push_back( {fsSpec, mountPoint, fsType, options } ); break; case 5: - fstabEntries.push_back( {splitLine.at(0), splitLine.at(1), splitLine.at(2), splitLine.at(3), splitLine.at(4).toInt() } ); + fstabEntries.push_back( {fsSpec, mountPoint, fsType, options, splitLine.at(4).toInt() } ); break; case 6: - fstabEntries.push_back( {splitLine.at(0), splitLine.at(1), splitLine.at(2), splitLine.at(3), splitLine.at(4).toInt(), splitLine.at(5).toInt(), comment.isEmpty() ? QString() : QLatin1Char('#') + comment } ); + fstabEntries.push_back( {fsSpec, mountPoint, fsType, options, splitLine.at(4).toInt(), splitLine.at(5).toInt(), comment.isEmpty() ? QString() : QLatin1Char('#') + comment } ); break; default: fstabEntries.push_back( { {}, {}, {}, {}, {}, {}, QLatin1Char('#') + line } ); From ba46ea64ba74666832e54896975b1a89bed44e4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrius=20=C5=A0tikonas?= Date: Mon, 14 Sep 2020 02:25:36 +0100 Subject: [PATCH 5/5] defaults option in fstab is not necessary, e.g. defaults,ro is the same as defaults. --- src/core/fstab.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/fstab.cpp b/src/core/fstab.cpp index e1b502a..e2bcbbd 100644 --- a/src/core/fstab.cpp +++ b/src/core/fstab.cpp @@ -63,6 +63,7 @@ FstabEntry::FstabEntry(const QString& fsSpec, const QString& mountPoint, const Q d->m_comment = comment; d->m_options = options.split(QLatin1Char(',')); + d->m_options.removeAll(QStringLiteral("defaults")); parseFsSpec(d->m_fsSpec, d->m_entryType, d->m_deviceNode); }