From f6f7789afad378c02e0b36f20a29b4423a55a750 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Wed, 9 Dec 2020 13:14:05 +0100 Subject: [PATCH 01/14] Log the final http request --- src/libsync/accessmanager.cpp | 3 +-- src/libsync/httplogger.cpp | 28 +++++++++++----------------- src/libsync/httplogger.h | 3 +-- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/libsync/accessmanager.cpp b/src/libsync/accessmanager.cpp index a93a6e342..db532a385 100644 --- a/src/libsync/accessmanager.cpp +++ b/src/libsync/accessmanager.cpp @@ -90,9 +90,8 @@ QNetworkReply *AccessManager::createRequest(QNetworkAccessManager::Operation op, } #endif - HttpLogger::logRequest(newRequest, op, outgoingData); const auto reply = QNetworkAccessManager::createRequest(op, newRequest, outgoingData); - HttpLogger::logReplyOnFinished(reply); + HttpLogger::logRequest(reply, op, outgoingData); return reply; } diff --git a/src/libsync/httplogger.cpp b/src/libsync/httplogger.cpp index d386bfbbc..85b2ebbf1 100644 --- a/src/libsync/httplogger.cpp +++ b/src/libsync/httplogger.cpp @@ -85,24 +85,9 @@ void logHttp(const QByteArray &verb, const QString &url, const QByteArray &id, c namespace OCC { - -void HttpLogger::logReplyOnFinished(const QNetworkReply *reply) -{ - if (!lcNetworkHttp().isInfoEnabled()) { - return; - } - QObject::connect(reply, &QNetworkReply::finished, reply, [reply] { - logHttp(requestVerb(*reply), - reply->url().toString(), - reply->request().rawHeader(XRequestId()), - reply->header(QNetworkRequest::ContentTypeHeader).toString(), - reply->rawHeaderPairs(), - const_cast(reply)); - }); -} - -void HttpLogger::logRequest(const QNetworkRequest &request, QNetworkAccessManager::Operation operation, QIODevice *device) +void HttpLogger::logRequest(QNetworkReply *reply, QNetworkAccessManager::Operation operation, QIODevice *device) { + const auto request = reply->request(); if (!lcNetworkHttp().isInfoEnabled()) { return; } @@ -118,6 +103,15 @@ void HttpLogger::logRequest(const QNetworkRequest &request, QNetworkAccessManage request.header(QNetworkRequest::ContentTypeHeader).toString(), header, device); + + QObject::connect(reply, &QNetworkReply::finished, reply, [reply] { + logHttp(requestVerb(*reply), + reply->url().toString(), + reply->request().rawHeader(XRequestId()), + reply->header(QNetworkRequest::ContentTypeHeader).toString(), + reply->rawHeaderPairs(), + reply); + }); } QByteArray HttpLogger::requestVerb(QNetworkAccessManager::Operation operation, const QNetworkRequest &request) diff --git a/src/libsync/httplogger.h b/src/libsync/httplogger.h index 1d8e59b68..2dde6c117 100644 --- a/src/libsync/httplogger.h +++ b/src/libsync/httplogger.h @@ -20,8 +20,7 @@ namespace OCC { namespace HttpLogger { - void OWNCLOUDSYNC_EXPORT logReplyOnFinished(const QNetworkReply *reply); - void OWNCLOUDSYNC_EXPORT logRequest(const QNetworkRequest &request, QNetworkAccessManager::Operation operation, QIODevice *device); + void OWNCLOUDSYNC_EXPORT logRequest(QNetworkReply *reply, QNetworkAccessManager::Operation operation, QIODevice *device); /** * Helper to construct the HTTP verb used in the request From 157d526129e10eeb4cb8ddeff3fa48870ba0c099 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Wed, 9 Dec 2020 15:39:01 +0100 Subject: [PATCH 02/14] Include auth type in http log --- src/libsync/httplogger.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libsync/httplogger.cpp b/src/libsync/httplogger.cpp index 85b2ebbf1..8f22bbe4b 100644 --- a/src/libsync/httplogger.cpp +++ b/src/libsync/httplogger.cpp @@ -53,7 +53,8 @@ void logHttp(const QByteArray &verb, const QString &url, const QByteArray &id, c for (const auto &it : header) { stream << it.first << ": "; if (it.first == "Authorization") { - stream << "[redacted]"; + stream << (it.second.startsWith("Bearer ") ? "Bearer" : "Basic"); + stream << " [redacted]"; } else { stream << it.second; } From 3f873ed2ee4de9b4e676bf53928757de90873f2b Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Mon, 14 Dec 2020 12:37:33 +0100 Subject: [PATCH 03/14] Simplify file comparison --- src/libsync/filesystem.cpp | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 4b68e6728..57452bc67 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -42,22 +42,17 @@ bool FileSystem::fileEquals(const QString &fn1, const QString &fn2) } const int BufferSize = 16 * 1024; - char buffer1[BufferSize]; - char buffer2[BufferSize]; - do { - int r = f1.read(buffer1, BufferSize); - if (f2.read(buffer2, BufferSize) != r) { - // this should normally not happen: the files are supposed to have the same size. + QByteArray buffer1(BufferSize, 0); + QByteArray buffer2(BufferSize, 0); + // the files have the same size, compare all of it + while(!f1.atEnd()){ + f1.read(buffer1.data(), BufferSize); + f2.read(buffer2.data(), BufferSize); + if (buffer1 != buffer2) { return false; } - if (r <= 0) { - return true; - } - if (memcmp(buffer1, buffer2, r) != 0) { - return false; - } - } while (true); - return false; + }; + return true; } time_t FileSystem::getModTime(const QString &filename) From 51d73e27eaffed31506f533e1e031569d8a953d1 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Mon, 14 Dec 2020 12:48:49 +0100 Subject: [PATCH 04/14] Clarify comment --- src/libsync/filesystem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 57452bc67..8f71d173b 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -125,7 +125,7 @@ qint64 FileSystem::getSize(const QString &filename) { #ifdef Q_OS_WIN if (isLnkFile(filename)) { - // Use csync to get the file size. Qt seems unable to get at it. + // Qt handles .lnk as symlink... https://doc.qt.io/qt-5/qfileinfo.html#details return getSizeWithCsync(filename); } #endif From 1144473f5dab7e9e1c9b97fa39d27094283e4167 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Mon, 14 Dec 2020 13:32:28 +0100 Subject: [PATCH 05/14] Cleanup pathtoUNC and its test --- src/common/filesystembase.cpp | 39 ++++---- src/common/filesystembase.h | 26 +++--- test/CMakeLists.txt | 1 + test/csync/CMakeLists.txt | 3 - test/csync/encoding_tests/check_encoding.cpp | 96 -------------------- test/testlongwinpath.cpp | 87 ++++++++++++++++++ 6 files changed, 117 insertions(+), 135 deletions(-) delete mode 100644 test/csync/encoding_tests/check_encoding.cpp create mode 100644 test/testlongwinpath.cpp diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 6b2271777..9914c1a7e 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -497,33 +497,26 @@ bool FileSystem::isJunction(const QString &filename) #endif } -QString FileSystem::pathtoUNC(const QString &str) +#ifdef Q_OS_WIN +QString FileSystem::pathtoUNC(const QString &_str) { - int len = 0; - QString longStr; - - len = str.length(); - longStr.reserve(len + 4); - - // prepend \\?\ and convert '/' => '\' to support long names - if (str[0] == QLatin1Char('/') || str[0] == QLatin1Char('\\')) { - // Don't prepend if already UNC - if (!(len > 1 && (str[1] == QLatin1Char('/') || str[1] == QLatin1Char('\\')))) { - longStr.append(QStringLiteral("\\\\?")); - } - } else { - longStr.append(QStringLiteral("\\\\?\\")); // prepend string by this four magic chars. + if (_str.isEmpty()) { + return _str; } - longStr += str; + const QString str = QDir::toNativeSeparators(_str); + const QLatin1Char sep('\\'); - /* replace all occurences of / with the windows native \ */ - - for (auto &c : longStr) { - if (c == QLatin1Char('/')) { - c = QLatin1Char('\\'); - } + // we already have a unc path + if (str.startsWith(sep + sep)) { + return str; } - return longStr; + // prepend \\?\ and to support long names + + if (str.at(0) == sep) { + return QStringLiteral(R"(\\?)") + str; + } + return QStringLiteral(R"(\\?\)") + str; } +#endif } // namespace OCC diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index 4e3573058..0259fb6c7 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -128,6 +128,19 @@ namespace FileSystem { * Returns the file system used at the given path. */ QString fileSystemForPath(const QString &path); + + /* + * This function takes a path and converts it to a UNC representation of the + * string. That means that it prepends a \\?\ (unless already UNC) and converts + * all slashes to backslashes. + * + * Note the following: + * - The string must be absolute. + * - it needs to contain a drive character to be a valid UNC + * - A conversion is only done if the path len is larger than 245. Otherwise + * the windows API functions work with the normal "unixoid" representation too. + */ + QString OCSYNC_EXPORT pathtoUNC(const QString &str); #endif /** @@ -144,19 +157,6 @@ namespace FileSystem { * Returns whether the file is a junction (windows only) */ bool OCSYNC_EXPORT isJunction(const QString &filename); - - /* - * This function takes a path and converts it to a UNC representation of the - * string. That means that it prepends a \\?\ (unless already UNC) and converts - * all slashes to backslashes. - * - * Note the following: - * - The string must be absolute. - * - it needs to contain a drive character to be a valid UNC - * - A conversion is only done if the path len is larger than 245. Otherwise - * the windows API functions work with the normal "unixoid" representation too. - */ - QString OCSYNC_EXPORT pathtoUNC(const QString &str); } /** @} */ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b13f6bff1..0d510c51a 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -74,6 +74,7 @@ if( UNIX AND NOT APPLE ) endif(UNIX AND NOT APPLE) if (WIN32) + nextcloud_add_test(LongWinPath "") nextcloud_add_test(SyncCfApi "") endif() diff --git a/test/csync/CMakeLists.txt b/test/csync/CMakeLists.txt index 224ecb606..27e6d3857 100644 --- a/test/csync/CMakeLists.txt +++ b/test/csync/CMakeLists.txt @@ -24,6 +24,3 @@ add_cmocka_test(check_std_c_jhash std_tests/check_std_c_jhash.c ${TEST_TARGET_LI # vio add_cmocka_test(check_vio_ext vio_tests/check_vio_ext.cpp ${TEST_TARGET_LIBRARIES}) -# encoding -add_cmocka_test(check_encoding_functions encoding_tests/check_encoding.cpp ${TEST_TARGET_LIBRARIES}) - diff --git a/test/csync/encoding_tests/check_encoding.cpp b/test/csync/encoding_tests/check_encoding.cpp deleted file mode 100644 index 98ac0fa22..000000000 --- a/test/csync/encoding_tests/check_encoding.cpp +++ /dev/null @@ -1,96 +0,0 @@ -/* - * libcsync -- a library to sync a directory with another - * - * Copyright (c) 2013 by Klaas Freitag - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - */ -#include -#include "common/filesystembase.h" -#include "torture.h" - -#ifdef _WIN32 -#include -#endif - -#include "torture.h" - -static void check_long_win_path(void **state) -{ - (void) state; /* unused */ - - { - const auto path = QStringLiteral("C://DATA/FILES/MUSIC/MY_MUSIC.mp3"); // check a short path - const auto exp_path = QStringLiteral(R"(\\?\C:\\DATA\FILES\MUSIC\MY_MUSIC.mp3)"); - QString new_short = OCC::FileSystem::pathtoUNC(path); - assert_string_equal(new_short.constData(), exp_path.constData()); - } - - { - const auto path = QStringLiteral(R"(\\foo\bar/MY_MUSIC.mp3)"); - const auto exp_path = QStringLiteral(R"(\\foo\bar\MY_MUSIC.mp3)"); - QString new_short = OCC::FileSystem::pathtoUNC(path); - assert_string_equal(new_short.constData(), exp_path.constData()); - } - - { - const auto path = QStringLiteral(R"(//foo\bar/MY_MUSIC.mp3)"); - const auto exp_path = QStringLiteral(R"(\\foo\bar\MY_MUSIC.mp3)"); - QString new_short = OCC::FileSystem::pathtoUNC(path); - assert_string_equal(new_short.constData(), exp_path.constData()); - } - - { - const auto path = QStringLiteral(R"(\foo\bar)"); - const auto exp_path = QStringLiteral(R"(\\?\foo\bar)"); - QString new_short = OCC::FileSystem::pathtoUNC(path); - assert_string_equal(new_short.constData(), exp_path.constData()); - } - - { - const auto path = QStringLiteral("/foo/bar"); - const auto exp_path = QStringLiteral(R"(\\?\foo\bar)"); - QString new_short = OCC::FileSystem::pathtoUNC(path); - assert_string_equal(new_short.constData(), exp_path.constData()); - } - - const auto longPath = QStringLiteral("D://alonglonglonglong/blonglonglonglong/clonglonglonglong/dlonglonglonglong/" - "elonglonglonglong/flonglonglonglong/glonglonglonglong/hlonglonglonglong/ilonglonglonglong/" - "jlonglonglonglong/klonglonglonglong/llonglonglonglong/mlonglonglonglong/nlonglonglonglong/" - "olonglonglonglong/file.txt"); - const auto longPathConv = QStringLiteral(R"(\\?\D:\\alonglonglonglong\blonglonglonglong\clonglonglonglong\dlonglonglonglong\)" - R"(elonglonglonglong\flonglonglonglong\glonglonglonglong\hlonglonglonglong\ilonglonglonglong\)" - R"(jlonglonglonglong\klonglonglonglong\llonglonglonglong\mlonglonglonglong\nlonglonglonglong\)" - R"(olonglonglonglong\file.txt)"); - - QString new_long = OCC::FileSystem::pathtoUNC(longPath); - // printf( "XXXXXXXXXXXX %s %d\n", new_long, mem_reserved); - - assert_string_equal(new_long.constData(), longPathConv.constData()); - - // printf( "YYYYYYYYYYYY %ld\n", strlen(new_long)); - assert_int_equal(new_long.length(), 286); -} - -int torture_run_tests(void) -{ - const struct CMUnitTest tests[] = { - cmocka_unit_test(check_long_win_path), - - }; - - return cmocka_run_group_tests(tests, nullptr, nullptr); -} - diff --git a/test/testlongwinpath.cpp b/test/testlongwinpath.cpp new file mode 100644 index 000000000..16933813c --- /dev/null +++ b/test/testlongwinpath.cpp @@ -0,0 +1,87 @@ +/* + * libcsync -- a library to sync a directory with another + * + * Copyright (c) 2013 by Klaas Freitag + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ +#include "common/filesystembase.h" + +#include + + +class TestLongWindowsPath : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + void check_long_win_path() + { + { + const auto path = QStringLiteral("C://DATA/FILES/MUSIC/MY_MUSIC.mp3"); // check a short path + const auto exp_path = QStringLiteral("\\\\?\\C:\\\\DATA\\FILES\\MUSIC\\MY_MUSIC.mp3"); + QString new_short = OCC::FileSystem::pathtoUNC(path); + QCOMPARE(new_short, exp_path); + } + + { + const auto path = QStringLiteral("\\\\foo\\bar/MY_MUSIC.mp3"); + const auto exp_path = QStringLiteral("\\\\foo\\bar\\MY_MUSIC.mp3"); + QString new_short = OCC::FileSystem::pathtoUNC(path); + QCOMPARE(new_short, exp_path); + } + + { + const auto path = QStringLiteral("//foo\\bar/MY_MUSIC.mp3"); + const auto exp_path = QStringLiteral("\\\\foo\\bar\\MY_MUSIC.mp3"); + QString new_short = OCC::FileSystem::pathtoUNC(path); + QCOMPARE(new_short, exp_path); + } + + { + const auto path = QStringLiteral("\\foo\\bar"); + const auto exp_path = QStringLiteral("\\\\?\\foo\\bar"); + QString new_short = OCC::FileSystem::pathtoUNC(path); + QCOMPARE(new_short, exp_path); + } + + { + const auto path = QStringLiteral("/foo/bar"); + const auto exp_path = QStringLiteral("\\\\?\\foo\\bar"); + QString new_short = OCC::FileSystem::pathtoUNC(path); + QCOMPARE(new_short, exp_path); + } + + const auto longPath = QStringLiteral("D://alonglonglonglong/blonglonglonglong/clonglonglonglong/dlonglonglonglong/" + "elonglonglonglong/flonglonglonglong/glonglonglonglong/hlonglonglonglong/ilonglonglonglong/" + "jlonglonglonglong/klonglonglonglong/llonglonglonglong/mlonglonglonglong/nlonglonglonglong/" + "olonglonglonglong/file.txt"); + const auto longPathConv = QStringLiteral("\\\\?\\D:\\\\alonglonglonglong\\blonglonglonglong\\clonglonglonglong\\dlonglonglonglong\\" + "elonglonglonglong\\flonglonglonglong\\glonglonglonglong\\hlonglonglonglong\\ilonglonglonglong\\" + "jlonglonglonglong\\klonglonglonglong\\llonglonglonglong\\mlonglonglonglong\\nlonglonglonglong\\" + "olonglonglonglong\\file.txt"); + + QString new_long = OCC::FileSystem::pathtoUNC(longPath); + // printf( "XXXXXXXXXXXX %s %d\n", new_long, mem_reserved); + + QCOMPARE(new_long, longPathConv); + + // printf( "YYYYYYYYYYYY %ld\n", strlen(new_long)); + QCOMPARE(new_long.length(), 286); + } +}; + +QTEST_GUILESS_MAIN(TestLongWindowsPath) +#include "testlongwinpath.moc" From 713a429675dad8ae8e6cddfa0737b1da8223b164 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Mon, 14 Dec 2020 13:35:05 +0100 Subject: [PATCH 06/14] Add todo for Qt 5.15 --- src/common/filesystembase.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 9914c1a7e..c42c8f700 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -372,6 +372,7 @@ bool FileSystem::remove(const QString &fileName, QString *errorString) bool FileSystem::moveToTrash(const QString &fileName, QString *errorString) { + // TODO: Qt 5.15 bool QFile::moveToTrash() #if defined Q_OS_UNIX && !defined Q_OS_MAC QString trashPath, trashFilePath, trashInfoPath; QString xdgDataHome = QFile::decodeName(qgetenv("XDG_DATA_HOME")); From 635d2b2da28336557b910d1bc6dbdf6a649c637b Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 14 Dec 2020 14:47:45 +0100 Subject: [PATCH 07/14] Fix style --- test/testutility.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/testutility.cpp b/test/testutility.cpp index 38acb0ca2..aa21ed788 100644 --- a/test/testutility.cpp +++ b/test/testutility.cpp @@ -114,22 +114,22 @@ private slots: void testVersionOfInstalledBinary() { - if( isLinux() ) { - if ( qgetenv("DISPLAY").isEmpty() ) { + if (isLinux()) { + if (qgetenv("DISPLAY").isEmpty()) { // Current requires an X-Server return; } // pass the binary name owncloud to the next call. This brakes branding, // but branding is not supposed to work with this. - QString ver = versionOfInstalledBinary(OWNCLOUD_BIN_PATH+QLatin1String("/nextcloud")); - qDebug() << "Version of installed Nextcloud: " << ver; - QVERIFY( !ver.isEmpty()); + QString ver = versionOfInstalledBinary(OWNCLOUD_BIN_PATH + QLatin1String("/nextcloud")); + qDebug() << "Version of installed Nextcloud: " << ver; + QVERIFY(!ver.isEmpty()); - QRegExp rx( R"(Nextcloud version \d+\.\d+\.\d+.*)" ); - QVERIFY( rx.exactMatch(ver)); - } else { - QVERIFY( versionOfInstalledBinary().isEmpty()); - } + QRegExp rx(R"(Nextcloud version \d+\.\d+\.\d+.*)"); + QVERIFY(rx.exactMatch(ver)); + } else { + QVERIFY(versionOfInstalledBinary().isEmpty()); + } } void testTimeAgo() From 67f98903203c3f42c65a62ba2057d12cf6c2d4fa Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 14 Dec 2020 15:37:05 +0100 Subject: [PATCH 08/14] Use owncloudcmd in testVersionOfInstalledBinary ... as it works without X in CI. --- test/testutility.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/testutility.cpp b/test/testutility.cpp index aa21ed788..eed1c2b90 100644 --- a/test/testutility.cpp +++ b/test/testutility.cpp @@ -8,6 +8,7 @@ #include #include "common/utility.h" +#include "config.h" using namespace OCC::Utility; @@ -115,13 +116,11 @@ private slots: void testVersionOfInstalledBinary() { if (isLinux()) { - if (qgetenv("DISPLAY").isEmpty()) { - // Current requires an X-Server - return; - } - // pass the binary name owncloud to the next call. This brakes branding, - // but branding is not supposed to work with this. - QString ver = versionOfInstalledBinary(OWNCLOUD_BIN_PATH + QLatin1String("/nextcloud")); + // pass the cmd client from our build dir + // this is a bit inaccurate as it does not test the "real thing" + // but cmd and gui have the same --version handler by now + // and cmd works without X in CI + QString ver = versionOfInstalledBinary(QStringLiteral(OWNCLOUD_BIN_PATH "/" APPLICATION_EXECUTABLE "cmd")); qDebug() << "Version of installed Nextcloud: " << ver; QVERIFY(!ver.isEmpty()); From 69915ab5942ab95c1c25ed9ecbaca2d67ac77522 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 14 Dec 2020 15:42:35 +0100 Subject: [PATCH 09/14] Fix testVersionOfInstalledBinary for brandings --- test/testutility.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testutility.cpp b/test/testutility.cpp index eed1c2b90..ad9f69b0c 100644 --- a/test/testutility.cpp +++ b/test/testutility.cpp @@ -124,7 +124,7 @@ private slots: qDebug() << "Version of installed Nextcloud: " << ver; QVERIFY(!ver.isEmpty()); - QRegExp rx(R"(Nextcloud version \d+\.\d+\.\d+.*)"); + QRegExp rx(APPLICATION_SHORTNAME R"( version \d+\.\d+\.\d+.*)"); QVERIFY(rx.exactMatch(ver)); } else { QVERIFY(versionOfInstalledBinary().isEmpty()); From 552427ffc3a1a997987f14dcd2ea0a62f3712202 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Tue, 15 Dec 2020 15:02:33 +0100 Subject: [PATCH 10/14] Update windows launch on start binary location Fixes: #7672 --- src/common/utility_win.cpp | 2 +- src/gui/generalsettings.cpp | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/common/utility_win.cpp b/src/common/utility_win.cpp index 8a56bcd41..c6f9c0066 100644 --- a/src/common/utility_win.cpp +++ b/src/common/utility_win.cpp @@ -94,7 +94,7 @@ void setLaunchOnStartup_private(const QString &appName, const QString &guiName, QString runPath = QLatin1String(runPathC); QSettings settings(runPath, QSettings::NativeFormat); if (enable) { - settings.setValue(appName, QCoreApplication::applicationFilePath().replace(QLatin1Char('/'), QLatin1Char('\\'))); + settings.setValue(appName, QDir::toNativeSeparators(QCoreApplication::applicationFilePath())); } else { settings.remove(appName); } diff --git a/src/gui/generalsettings.cpp b/src/gui/generalsettings.cpp index 1e87d7ae6..0f4c21f52 100644 --- a/src/gui/generalsettings.cpp +++ b/src/gui/generalsettings.cpp @@ -161,7 +161,10 @@ GeneralSettings::GeneralSettings(QWidget *parent) _ui->autostartCheckBox->setDisabled(true); _ui->autostartCheckBox->setToolTip(tr("You cannot disable autostart because system-wide autostart is enabled.")); } else { - _ui->autostartCheckBox->setChecked(Utility::hasLaunchOnStartup(Theme::instance()->appName())); + const bool hasAutoStart = Utility::hasLaunchOnStartup(Theme::instance()->appName()); + // make sure the binary location is correctly set + slotToggleLaunchOnStartup(hasAutoStart); + _ui->autostartCheckBox->setChecked(hasAutoStart); connect(_ui->autostartCheckBox, &QAbstractButton::toggled, this, &GeneralSettings::slotToggleLaunchOnStartup); } From aadda3263381174585a4208be150e98051e3c304 Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Wed, 30 Dec 2020 12:53:03 +0100 Subject: [PATCH 11/14] Handle errors in convertToPlaceholder --- src/common/vfs.h | 4 ++-- src/libsync/owncloudpropagator.cpp | 4 +++- src/libsync/propagatedownload.cpp | 6 +++++- src/libsync/syncengine.cpp | 7 ++++++- src/libsync/vfs/cfapi/vfs_cfapi.cpp | 6 +++--- src/libsync/vfs/cfapi/vfs_cfapi.h | 2 +- src/libsync/vfs/suffix/vfs_suffix.cpp | 3 ++- src/libsync/vfs/suffix/vfs_suffix.h | 2 +- 8 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/common/vfs.h b/src/common/vfs.h index f4ea3d8fa..b7bd22098 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -190,7 +190,7 @@ public: * new placeholder shall supersede, for rename-replace actions with new downloads, * for example. */ - virtual void convertToPlaceholder( + virtual Result convertToPlaceholder( const QString &filename, const SyncFileItem &item, const QString &replacesFile = QString()) = 0; @@ -296,7 +296,7 @@ public: Result updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; } Result createPlaceholder(const SyncFileItem &) override { return {}; } Result dehydratePlaceholder(const SyncFileItem &) override { return {}; } - void convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override {} + Result convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override { return {}; } bool needsMetadataUpdate(const SyncFileItem &) override { return false; } bool isDehydratedPlaceholder(const QString &) override { return false; } diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index f26dad05c..c576c93df 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -737,7 +737,9 @@ QString OwncloudPropagator::adjustRenamedPath(const QString &original) const bool OwncloudPropagator::updateMetadata(const SyncFileItem &item, const QString &localFolderPath, SyncJournalDb &journal, Vfs &vfs) { QString fsPath = localFolderPath + item.destination(); - vfs.convertToPlaceholder(fsPath, item); + if (!vfs.convertToPlaceholder(fsPath, item)) { + return false; + } auto record = item.toSyncJournalFileRecordWithInode(fsPath); return journal.setFileRecord(record); } diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 161b191df..06c0c6337 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -964,7 +964,11 @@ void PropagateDownloadFile::downloadFinished() preserveGroupOwnership(_tmpFile.fileName(), existingFile); // Make the file a hydrated placeholder if possible - propagator()->syncOptions()._vfs->convertToPlaceholder(_tmpFile.fileName(), *_item, fn); + const auto result = propagator()->syncOptions()._vfs->convertToPlaceholder(_tmpFile.fileName(), *_item, fn); + if (!result) { + done(SyncFileItem::NormalError, result.error()); + return; + } } // Apply the remote permissions diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 9c2dd8edc..7d6ae74d3 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -346,7 +346,12 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) // Ensure it's a placeholder file on disk if (item->_type == ItemTypeFile) { - _syncOptions._vfs->convertToPlaceholder(filePath, *item); + const auto result = _syncOptions._vfs->convertToPlaceholder(filePath, *item); + if (!result) { + item->_instruction = CSYNC_INSTRUCTION_ERROR; + item->_errorString = tr("Could not update file : %1").arg(result.error()); + return; + } } // Update on-disk virtual file metadata diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.cpp b/src/libsync/vfs/cfapi/vfs_cfapi.cpp index 4a73ad968..7a5da1e0e 100644 --- a/src/libsync/vfs/cfapi/vfs_cfapi.cpp +++ b/src/libsync/vfs/cfapi/vfs_cfapi.cpp @@ -149,16 +149,16 @@ Result VfsCfApi::dehydratePlaceholder(const SyncFileItem &item) return {}; } -void VfsCfApi::convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) +Result VfsCfApi::convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) { const auto localPath = QDir::toNativeSeparators(filename); const auto replacesPath = QDir::toNativeSeparators(replacesFile); const auto handle = cfapi::handleForPath(localPath); if (cfapi::findPlaceholderInfo(handle)) { - cfapi::updatePlaceholderInfo(handle, item._modtime, item._size, item._fileId, replacesPath); + return cfapi::updatePlaceholderInfo(handle, item._modtime, item._size, item._fileId, replacesPath); } else { - cfapi::convertToPlaceholder(handle, item._modtime, item._size, item._fileId, replacesPath); + return cfapi::convertToPlaceholder(handle, item._modtime, item._size, item._fileId, replacesPath); } } diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.h b/src/libsync/vfs/cfapi/vfs_cfapi.h index e4a4d0074..96dda52d4 100644 --- a/src/libsync/vfs/cfapi/vfs_cfapi.h +++ b/src/libsync/vfs/cfapi/vfs_cfapi.h @@ -43,7 +43,7 @@ public: Result createPlaceholder(const SyncFileItem &item) override; Result dehydratePlaceholder(const SyncFileItem &item) override; - void convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) override; + Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) override; bool needsMetadataUpdate(const SyncFileItem &) override; bool isDehydratedPlaceholder(const QString &filePath) override; diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp index 40a7519a7..f3858e8a2 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.cpp +++ b/src/libsync/vfs/suffix/vfs_suffix.cpp @@ -122,9 +122,10 @@ Result VfsSuffix::dehydratePlaceholder(const SyncFileItem &item) return {}; } -void VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) +Result VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) { // Nothing necessary + return {}; } bool VfsSuffix::isDehydratedPlaceholder(const QString &filePath) diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h index 3feca6ea0..4aa513c03 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.h +++ b/src/libsync/vfs/suffix/vfs_suffix.h @@ -41,7 +41,7 @@ public: Result createPlaceholder(const SyncFileItem &item) override; Result dehydratePlaceholder(const SyncFileItem &item) override; - void convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override; + Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override; bool needsMetadataUpdate(const SyncFileItem &) override { return false; } bool isDehydratedPlaceholder(const QString &filePath) override; From 72b6118c3eae93d093d4bd1cb6a7086b7cf15eca Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Thu, 17 Dec 2020 14:13:24 +0100 Subject: [PATCH 12/14] Mark vfs functions as OC_REQUIRED_RESULT --- src/common/asserts.h | 7 +++++++ src/common/vfs.h | 21 +++++++++++---------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/common/asserts.h b/src/common/asserts.h index 38440ce1b..1967717e4 100644 --- a/src/common/asserts.h +++ b/src/common/asserts.h @@ -3,6 +3,13 @@ #include +#if 0 && defined(Q_CC_MSVC) +// requires c++2017 +#define OC_REQUIRED_RESULT [[nodiscard]] +#else +#define OC_REQUIRED_RESULT Q_REQUIRED_RESULT +#endif + #if defined(QT_FORCE_ASSERTS) || !defined(QT_NO_DEBUG) #define OC_ASSERT_MSG qFatal #else diff --git a/src/common/vfs.h b/src/common/vfs.h index b7bd22098..c4e9ffa37 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -20,6 +20,7 @@ #include +#include "assert.h" #include "ocsynclib.h" #include "result.h" #include "syncfilestatus.h" @@ -158,24 +159,24 @@ public: * If the remote metadata changes, the local placeholder's metadata should possibly * change as well. */ - virtual Result updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) = 0; + virtual OC_REQUIRED_RESULT Result updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) = 0; /// Create a new dehydrated placeholder. Called from PropagateDownload. - virtual Result createPlaceholder(const SyncFileItem &item) = 0; + virtual OC_REQUIRED_RESULT Result createPlaceholder(const SyncFileItem &item) = 0; /** Convert a hydrated placeholder to a dehydrated one. Called from PropagateDownlaod. * * This is different from delete+create because preserving some file metadata * (like pin states) may be essential for some vfs plugins. */ - virtual Result dehydratePlaceholder(const SyncFileItem &item) = 0; + virtual OC_REQUIRED_RESULT Result dehydratePlaceholder(const SyncFileItem &item) = 0; /** Discovery hook: even unchanged files may need UPDATE_METADATA. * * For instance cfapi vfs wants local hydrated non-placeholder files to * become hydrated placeholder files. */ - virtual bool needsMetadataUpdate(const SyncFileItem &item) = 0; + virtual OC_REQUIRED_RESULT bool needsMetadataUpdate(const SyncFileItem &item) = 0; /** Convert a new file to a hydrated placeholder. * @@ -190,13 +191,13 @@ public: * new placeholder shall supersede, for rename-replace actions with new downloads, * for example. */ - virtual Result convertToPlaceholder( + virtual OC_REQUIRED_RESULT Result convertToPlaceholder( const QString &filename, const SyncFileItem &item, const QString &replacesFile = QString()) = 0; /// Determine whether the file at the given absolute path is a dehydrated placeholder. - virtual bool isDehydratedPlaceholder(const QString &filePath) = 0; + virtual OC_REQUIRED_RESULT bool isDehydratedPlaceholder(const QString &filePath) = 0; /** Similar to isDehydratedPlaceholder() but used from sync discovery. * @@ -205,7 +206,7 @@ public: * * Returning true means that type was fully determined. */ - virtual bool statTypeVirtualFile(csync_file_stat_t *stat, void *stat_data) = 0; + virtual OC_REQUIRED_RESULT bool statTypeVirtualFile(csync_file_stat_t *stat, void *stat_data) = 0; /** Sets the pin state for the item at a path. * @@ -216,7 +217,7 @@ public: * * folderPath is relative to the sync folder. Can be "" for root folder. */ - virtual bool setPinState(const QString &folderPath, PinState state) = 0; + virtual OC_REQUIRED_RESULT bool setPinState(const QString &folderPath, PinState state) = 0; /** Returns the pin state of an item at a path. * @@ -227,7 +228,7 @@ public: * * Returns none on retrieval error. */ - virtual Optional pinState(const QString &folderPath) = 0; + virtual OC_REQUIRED_RESULT Optional pinState(const QString &folderPath) = 0; /** Returns availability status of an item at a path. * @@ -236,7 +237,7 @@ public: * * folderPath is relative to the sync folder. Can be "" for root folder. */ - virtual AvailabilityResult availability(const QString &folderPath) = 0; + virtual OC_REQUIRED_RESULT AvailabilityResult availability(const QString &folderPath) = 0; public slots: /** Update in-sync state based on SyncFileStatusTracker signal. From da3ff631efef2e6df0e91f77fffc0256874bdd87 Mon Sep 17 00:00:00 2001 From: Kevin Ottens Date: Wed, 30 Dec 2020 13:46:14 +0100 Subject: [PATCH 13/14] Use Q_REQUIRED_RESULT directly like in other places Signed-off-by: Kevin Ottens --- src/common/asserts.h | 7 ------- src/common/vfs.h | 21 ++++++++++----------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/common/asserts.h b/src/common/asserts.h index 1967717e4..38440ce1b 100644 --- a/src/common/asserts.h +++ b/src/common/asserts.h @@ -3,13 +3,6 @@ #include -#if 0 && defined(Q_CC_MSVC) -// requires c++2017 -#define OC_REQUIRED_RESULT [[nodiscard]] -#else -#define OC_REQUIRED_RESULT Q_REQUIRED_RESULT -#endif - #if defined(QT_FORCE_ASSERTS) || !defined(QT_NO_DEBUG) #define OC_ASSERT_MSG qFatal #else diff --git a/src/common/vfs.h b/src/common/vfs.h index c4e9ffa37..77501a67e 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -20,7 +20,6 @@ #include -#include "assert.h" #include "ocsynclib.h" #include "result.h" #include "syncfilestatus.h" @@ -159,24 +158,24 @@ public: * If the remote metadata changes, the local placeholder's metadata should possibly * change as well. */ - virtual OC_REQUIRED_RESULT Result updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) = 0; + virtual Q_REQUIRED_RESULT Result updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) = 0; /// Create a new dehydrated placeholder. Called from PropagateDownload. - virtual OC_REQUIRED_RESULT Result createPlaceholder(const SyncFileItem &item) = 0; + virtual Q_REQUIRED_RESULT Result createPlaceholder(const SyncFileItem &item) = 0; /** Convert a hydrated placeholder to a dehydrated one. Called from PropagateDownlaod. * * This is different from delete+create because preserving some file metadata * (like pin states) may be essential for some vfs plugins. */ - virtual OC_REQUIRED_RESULT Result dehydratePlaceholder(const SyncFileItem &item) = 0; + virtual Q_REQUIRED_RESULT Result dehydratePlaceholder(const SyncFileItem &item) = 0; /** Discovery hook: even unchanged files may need UPDATE_METADATA. * * For instance cfapi vfs wants local hydrated non-placeholder files to * become hydrated placeholder files. */ - virtual OC_REQUIRED_RESULT bool needsMetadataUpdate(const SyncFileItem &item) = 0; + virtual Q_REQUIRED_RESULT bool needsMetadataUpdate(const SyncFileItem &item) = 0; /** Convert a new file to a hydrated placeholder. * @@ -191,13 +190,13 @@ public: * new placeholder shall supersede, for rename-replace actions with new downloads, * for example. */ - virtual OC_REQUIRED_RESULT Result convertToPlaceholder( + virtual Q_REQUIRED_RESULT Result convertToPlaceholder( const QString &filename, const SyncFileItem &item, const QString &replacesFile = QString()) = 0; /// Determine whether the file at the given absolute path is a dehydrated placeholder. - virtual OC_REQUIRED_RESULT bool isDehydratedPlaceholder(const QString &filePath) = 0; + virtual Q_REQUIRED_RESULT bool isDehydratedPlaceholder(const QString &filePath) = 0; /** Similar to isDehydratedPlaceholder() but used from sync discovery. * @@ -206,7 +205,7 @@ public: * * Returning true means that type was fully determined. */ - virtual OC_REQUIRED_RESULT bool statTypeVirtualFile(csync_file_stat_t *stat, void *stat_data) = 0; + virtual Q_REQUIRED_RESULT bool statTypeVirtualFile(csync_file_stat_t *stat, void *stat_data) = 0; /** Sets the pin state for the item at a path. * @@ -217,7 +216,7 @@ public: * * folderPath is relative to the sync folder. Can be "" for root folder. */ - virtual OC_REQUIRED_RESULT bool setPinState(const QString &folderPath, PinState state) = 0; + virtual Q_REQUIRED_RESULT bool setPinState(const QString &folderPath, PinState state) = 0; /** Returns the pin state of an item at a path. * @@ -228,7 +227,7 @@ public: * * Returns none on retrieval error. */ - virtual OC_REQUIRED_RESULT Optional pinState(const QString &folderPath) = 0; + virtual Q_REQUIRED_RESULT Optional pinState(const QString &folderPath) = 0; /** Returns availability status of an item at a path. * @@ -237,7 +236,7 @@ public: * * folderPath is relative to the sync folder. Can be "" for root folder. */ - virtual OC_REQUIRED_RESULT AvailabilityResult availability(const QString &folderPath) = 0; + virtual Q_REQUIRED_RESULT AvailabilityResult availability(const QString &folderPath) = 0; public slots: /** Update in-sync state based on SyncFileStatusTracker signal. From 09cc988026f37f9f9a525437c418c02207f5493b Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Fri, 18 Dec 2020 12:48:08 +0100 Subject: [PATCH 14/14] Fix a possible crash with the remove all files dialog --- src/gui/folder.cpp | 1 + src/libsync/syncengine.cpp | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 742981195..4f8c1e152 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -1260,6 +1260,7 @@ void Folder::slotAboutToRemoveAllFiles(SyncFileItem::Direction dir, std::functio } setSyncPaused(oldPaused); }); + connect(this, &Folder::destroyed, msgBox, &QMessageBox::deleteLater); msgBox->open(); } diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 7d6ae74d3..89d4057fc 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -737,10 +737,11 @@ void SyncEngine::slotDiscoveryFinished() } QPointer guard = new QObject(); - auto callback = [this, finish, guard](bool cancel) -> void { + QPointer self = this; + auto callback = [this, self, finish, guard](bool cancel) -> void { // use a guard to ensure its only called once... - if (!guard) - { + // qpointer to self to ensure we still exist + if (!guard || !self) { return; } guard->deleteLater();