Skip to content

Commit f25acfd

Browse files
dragotindragotin
committed
Various review feedback from @TheOneRing 
1 parent de997f0 commit f25acfd

File tree

3 files changed

+79
-106
lines changed

3 files changed

+79
-106
lines changed

‎src/libsync/vfs/vfs.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,9 +253,9 @@ public Q_SLOTS:
253253
*/
254254
virtual void startImpl(const VfsSetupParams &params) = 0;
255255

256+
private:
256257
// the parameters passed to start()
257258
std::unique_ptr<VfsSetupParams> _setupParams;
258-
private:
259259

260260
friend class OwncloudPropagator;
261261
};

‎src/plugins/vfs/xattr/vfs_xattr.cpp‎

Lines changed: 77 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -12,25 +12,38 @@
1212
#include "libsync/xattr.h"
1313
#include "syncfileitem.h"
1414
#include "vfs/hydrationjob.h"
15+
#include "theme.h"
16+
17+
#include <string>
1518

1619
#include <QDir>
1720
#include <QFile>
1821
#include <QLocalSocket>
1922
#include <QLoggingCategory>
2023
#include <QUuid>
24+
#include <QString>
25+
26+
using namespace Qt::StringLiterals;
27+
using namespace xattr;
2128

2229
Q_LOGGING_CATEGORY(lcVfsXAttr, "sync.vfs.xattr", QtInfoMsg)
2330

2431

2532
namespace {
26-
const QString ownerXAttrName = QStringLiteral("user.openvfs.owner");
27-
const QString etagXAttrName = QStringLiteral("user.openvfs.etag");
28-
const QString fileidXAttrName = QStringLiteral("user.openvfs.fileid");
29-
const QString modtimeXAttrName = QStringLiteral("user.openvfs.modtime");
30-
const QString fileSizeXAttrName = QStringLiteral("user.openvfs.fsize");
31-
const QString actionXAttrName = QStringLiteral("user.openvfs.action");
32-
const QString stateXAttrName = QStringLiteral("user.openvfs.state");
33-
const QString pinstateXAttrName = QStringLiteral("user.openvfs.pinstate");
33+
const QString ownerXAttrName = u"user.openvfs.owner"_s;
34+
const QString etagXAttrName = u"user.openvfs.etag"_s;
35+
const QString fileidXAttrName = u"user.openvfs.fileid"_s;
36+
const QString modtimeXAttrName = u"user.openvfs.modtime"_s;
37+
const QString fileSizeXAttrName = u"user.openvfs.fsize"_s;
38+
const QString actionXAttrName = u"user.openvfs.action"_s;
39+
const QString stateXAttrName = u"user.openvfs.state"_s;
40+
const QString pinstateXAttrName = u"user.openvfs.pinstate"_s;
41+
42+
const QString fileStateVirtual = u"virtual"_s;
43+
const QString fileStateHydrate = u"hydrate"_s;
44+
const QString fileStateDehydrate = u"dehydrate"_s;
45+
const QString fileStateHydrated = u"hydrated"_s;
46+
3447
}
3548

3649
namespace OCC {
@@ -49,10 +62,7 @@ Vfs::Mode VfsXAttr::mode() const
4962

5063
QString VfsXAttr::xattrOwnerString() const
5164
{
52-
auto s = QByteArray(APPLICATION_EXECUTABLE);
53-
s.append(":");
54-
s.append(_setupParams->account->uuid().toByteArray(QUuid::WithoutBraces));
55-
return QString::fromUtf8(s);
65+
return u"%1:%2"_s.arg(Theme::instance()->appName(), params().account->uuid().toString(QUuid::WithoutBraces));
5666
}
5767

5868
void VfsXAttr::startImpl(const VfsSetupParams &params)
@@ -68,23 +78,18 @@ void VfsXAttr::startImpl(const VfsSetupParams &params)
6878
if (!owner) {
6979
// set the owner to opencloud to claim it
7080
if (!FileSystem::Xattr::setxattr(path, ownerXAttrName, xattrOwnerString() )) {
71-
err = Vfs::tr("Unable to claim the sync root for files on demand");
72-
return;
81+
err = tr("Unable to claim the sync root for files on demand");
7382
}
74-
} else {
83+
} else if (owner.value() != xattrOwnerString()) {
7584
// owner is set. See if it is us
76-
if (owner.value() == xattrOwnerString()) {
77-
// all good
78-
} else {
79-
qCDebug(lcVfsXAttr) << "Root-FS has a different owner" << owner.value() << "Not our vfs!";
80-
err = Vfs::tr("The sync path is claimed by a different cloud, please check your setup");
81-
return;
82-
}
85+
qCDebug(lcVfsXAttr) << "Root-FS has a different owner" << owner.value() << "Not our vfs!";
86+
err = tr("The sync path is claimed by a different cloud, please check your setup");
8387
}
84-
if (err.isEmpty())
88+
if (err.isEmpty()) {
8589
Q_EMIT started();
86-
else
90+
} else {
8791
Q_EMIT error(err);
92+
}
8893
}
8994

9095
void VfsXAttr::stop()
@@ -100,13 +105,11 @@ bool VfsXAttr::socketApiPinStateActionsShown() const
100105
return true;
101106
}
102107

103-
PlaceHolderAttribs VfsXAttr::placeHolderAttributes(const QString& path)
108+
xattr::PlaceHolderAttribs VfsXAttr::placeHolderAttributes(const QString& path)
104109
{
105110
PlaceHolderAttribs attribs;
106111
const auto fPath = FileSystem::toFilesystemPath(path);
107112

108-
// lambda to handle the Optional return val of xattrGet
109-
110113
attribs._etag = FileSystem::Xattr::getxattr(fPath, etagXAttrName).value_or(QString());
111114
attribs._fileId = FileSystem::Xattr::getxattr(fPath, fileidXAttrName).value_or(QString());
112115

@@ -127,7 +130,7 @@ OCC::Result<void, QString> VfsXAttr::addPlaceholderAttribute(const QString &path
127130
auto success = FileSystem::Xattr::setxattr(FileSystem::toFilesystemPath(path), name, value);
128131
// Q_ASSERT(success);
129132
if (!success) {
130-
return Vfs::tr("Failed to set the extended file attribute");
133+
return tr("Failed to set the extended file attribute");
131134
}
132135
}
133136

@@ -150,17 +153,17 @@ OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> VfsXAttr::updateMetad
150153
if (!r) {
151154
res = OCC::Vfs::ConvertToPlaceholderResult::Locked;
152155
}
153-
addPlaceholderAttribute(filePath, actionXAttrName, QStringLiteral("dehydrate"));
154-
addPlaceholderAttribute(filePath, stateXAttrName, QStringLiteral("virtual"));
156+
addPlaceholderAttribute(filePath, actionXAttrName, fileStateDehydrate);
157+
addPlaceholderAttribute(filePath, stateXAttrName, fileStateVirtual);
155158
addPlaceholderAttribute(filePath, fileSizeXAttrName, QString::number(syncItem._size));
156159
} else if (syncItem._type == ItemTypeVirtualFileDownload) {
157-
addPlaceholderAttribute(filePath, actionXAttrName, QStringLiteral("hydrate"));
160+
addPlaceholderAttribute(filePath, actionXAttrName, fileStateHydrate);
158161
// file gets downloaded and becomes a normal file, the xattr gets removed
159162
FileSystem::Xattr::removexattr(localPath, stateXAttrName);
160163
FileSystem::Xattr::removexattr(localPath, fileSizeXAttrName);
161164
} else if (syncItem._type == ItemTypeVirtualFile) {
162165
qCDebug(lcVfsXAttr) << "updateMetadata for virtual file " << syncItem._type;
163-
addPlaceholderAttribute(filePath, stateXAttrName, QStringLiteral("virtual"));
166+
addPlaceholderAttribute(filePath, stateXAttrName, fileStateVirtual);
164167
addPlaceholderAttribute(filePath, fileSizeXAttrName, QString::number(syncItem._size));
165168
} else if (syncItem._type == ItemTypeFile) {
166169
qCDebug(lcVfsXAttr) << "updateMetadata for normal file " << syncItem._type;
@@ -205,36 +208,26 @@ void VfsXAttr::slotHydrateJobFinished()
205208

206209
// set the xattrs
207210
// the file is not virtual any more, remove the xattrs. No state xattr means local available data
208-
bool ok{true};
209-
ok = ok && FileSystem::Xattr::removexattr(targetPath, stateXAttrName);
210-
if (!ok) {
211-
qCInfo(lcVfsXAttr) << u"Removing extended file attribute state failed for" << targetPath;
212-
}
213-
ok = ok && FileSystem::Xattr::removexattr(targetPath, actionXAttrName);
214-
ok = ok && FileSystem::Xattr::removexattr(targetPath, fileSizeXAttrName);
215-
if (!ok) {
211+
if (!(FileSystem::Xattr::removexattr(targetPath, stateXAttrName) &&
212+
FileSystem::Xattr::removexattr(targetPath, actionXAttrName) &&
213+
FileSystem::Xattr::removexattr(targetPath, fileSizeXAttrName) )) {
216214
qCInfo(lcVfsXAttr) << u"Removing extended file attribute action failed for" << targetPath;
217215
}
218216

219-
if (ok) {
220-
time_t modtime = item->_modtime;
221-
qCInfo(lcVfsXAttr) << u"Setting hydrated file's modtime to" << modtime;
217+
time_t modtime = item->_modtime;
218+
qCInfo(lcVfsXAttr) << u"Setting hydrated file's modtime to" << modtime;
222219

223-
if (!FileSystem::setModTime(targetPath, modtime)) {
224-
qCInfo(lcVfsXAttr) << u"Failed to set the mod time of the hydrated file" << targetPath;
225-
// What can be done in this error condition
226-
ok = false;
227-
}
220+
if (!FileSystem::setModTime(targetPath, modtime)) {
221+
qCInfo(lcVfsXAttr) << u"Failed to set the mod time of the hydrated file" << targetPath;
222+
// What can be done in this error condition
228223
}
229224

230-
if (ok) {
231-
// Update the client sync journal database if the file modifications have been successful
232-
const auto result = this->params().journal->setFileRecord(SyncJournalFileRecord::fromSyncFileItem(*item));
233-
if (!result) {
234-
qCWarning(lcVfsXAttr) << u"Error when setting the file record to the database" << result.error();
235-
} else {
236-
qCInfo(lcVfsXAttr) << u"Hydration succeeded" << targetPath;
237-
}
225+
// Update the client sync journal database if the file modifications have been successful
226+
const auto result = this->params().journal->setFileRecord(SyncJournalFileRecord::fromSyncFileItem(*item));
227+
if (!result) {
228+
qCWarning(lcVfsXAttr) << u"Error when setting the file record to the database" << result.error();
229+
} else {
230+
qCInfo(lcVfsXAttr) << u"Hydration succeeded" << targetPath;
238231
}
239232
} else {
240233
qCWarning(lcVfsXAttr) << u"Hydration succeeded but the file appears to be moved" << targetPath;
@@ -254,7 +247,7 @@ Result<void, QString> VfsXAttr::createPlaceholder(const SyncFileItem &item)
254247
if (file.exists()
255248
&& FileSystem::fileChanged(FileSystem::toFilesystemPath(path),
256249
FileSystem::FileChangedInfo::fromSyncFileItem(&item))) {
257-
return Vfs::tr("Cannot create a placeholder because a file with the placeholder name already exist");
250+
return tr("Cannot create a placeholder because a file with the placeholder name already exist");
258251
}
259252

260253
if (!file.open(QFile::ReadWrite | QFile::Truncate)) {
@@ -267,7 +260,7 @@ Result<void, QString> VfsXAttr::createPlaceholder(const SyncFileItem &item)
267260

268261
// FIXME only write attribs if they're different, and/or all together
269262
addPlaceholderAttribute(path, fileSizeXAttrName, QString::number(item._size));
270-
addPlaceholderAttribute(path, stateXAttrName, QStringLiteral("virtual"));
263+
addPlaceholderAttribute(path, stateXAttrName, fileStateVirtual);
271264
addPlaceholderAttribute(path, fileidXAttrName, QString::fromUtf8(item._fileId));
272265
addPlaceholderAttribute(path, etagXAttrName, item._etag);
273266
FileSystem::setModTime(path, item._modtime);
@@ -294,7 +287,7 @@ HydrationJob* VfsXAttr::hydrateFile(const QByteArray &fileId, const QString &tar
294287
_hydrationJobs.insert(fileId, hydration);
295288

296289
// set an action attrib
297-
addPlaceholderAttribute(targetPath, actionXAttrName, QStringLiteral("hydrate"));
290+
addPlaceholderAttribute(targetPath, actionXAttrName, fileStateHydrate);
298291

299292
connect(hydration, &HydrationJob::finished, this, &VfsXAttr::slotHydrateJobFinished);
300293

@@ -309,28 +302,19 @@ HydrationJob* VfsXAttr::hydrateFile(const QByteArray &fileId, const QString &tar
309302

310303
bool VfsXAttr::needsMetadataUpdate(const SyncFileItem &item)
311304
{
312-
// return true if file exists
313-
const auto path = item.localName();
314-
QFileInfo fi{path};
315-
316-
// FIXME: Unsure about this implementation
317-
bool re{false};
318-
if (fi.exists()) {
319-
re = true;
320-
}
321-
qCDebug(lcVfsXAttr) << "returning" << re;
322-
return re;
305+
const QString path = params().filesystemPath + item.localName();
306+
307+
return QFileInfo::exists(path);
323308
}
324309

325310
bool VfsXAttr::isDehydratedPlaceholder(const QString &filePath)
326311
{
327-
const auto fi = QFileInfo(filePath);
328-
bool re{false};
329-
if (fi.exists()) {
312+
313+
if (QFileInfo::exists(filePath)) {
330314
const auto attribs = placeHolderAttributes(filePath);
331-
re = (attribs.state() == QStringLiteral("virtual"));
315+
return (attribs.state() == fileStateVirtual);
332316
}
333-
return re;
317+
return false;
334318
}
335319

336320
LocalInfo VfsXAttr::statTypeVirtualFile(const std::filesystem::directory_entry &path, ItemType type)
@@ -339,7 +323,7 @@ LocalInfo VfsXAttr::statTypeVirtualFile(const std::filesystem::directory_entry &
339323
if (type == ItemTypeFile) {
340324

341325
auto attribs = placeHolderAttributes(p);
342-
if (attribs.state() == QStringLiteral("virtual")) {
326+
if (attribs.state() == fileStateVirtual) {
343327
type = ItemTypeVirtualFile;
344328
if (attribs.pinState() == pinStateToString(PinState::AlwaysLocal)) {
345329
type = ItemTypeVirtualFileDownload;
@@ -435,42 +419,35 @@ void VfsXAttr::fileStatusChanged(const QString& systemFileName, SyncFileStatus f
435419

436420
QString VfsXAttr::pinStateToString(PinState pState) const
437421
{
438-
QString re;
439422
switch (pState) {
440423
case OCC::PinState::AlwaysLocal:
441-
re = QStringLiteral("alwayslocal");
442-
break;
424+
return u"alwayslocal"_s;
443425
case OCC::PinState::Inherited:
444-
re = QStringLiteral("interited");
445-
break;
426+
return u"interited"_s;
446427
case OCC::PinState::OnlineOnly:
447-
re = QStringLiteral("onlineonly");
448-
break;
428+
return u"onlineonly"_s;
449429
case OCC::PinState::Unspecified:
450-
re = QStringLiteral("unspecified");
451-
break;
430+
return u"unspecified"_s;
452431
case OCC::PinState::Excluded:
453-
re = QStringLiteral("excluded");
454-
break;
432+
return u"excluded"_s;
455433
};
456-
return re;
434+
return u"unspecified"_s;
457435
}
458436

459437
PinState VfsXAttr::stringToPinState(const QString& str) const
460438
{
461-
PinState p{PinState::Unspecified};
462-
if (str.isEmpty() || str == QStringLiteral("unspecified")) {
463-
p = PinState::Unspecified;
464-
} else if( str == QStringLiteral("alwayslocal")) {
465-
p = PinState::AlwaysLocal;
466-
} else if( str == QStringLiteral("inherited")) {
467-
p = PinState::Inherited;
468-
} else if( str == QStringLiteral("unspecified")) {
469-
p = PinState::Unspecified;
470-
} else if( str == QStringLiteral("excluded")) {
471-
p = PinState::Excluded;
439+
if (str.isEmpty() || str == u"unspecified"_s) {
440+
return PinState::Unspecified;
441+
} else if( str == u"alwayslocal"_s) {
442+
return PinState::AlwaysLocal;
443+
} else if( str == u"inherited"_s) {
444+
return PinState::Inherited;
445+
} else if( str == u"unspecified"_s) {
446+
return PinState::Unspecified;
447+
} else if( str == u"excluded"_s) {
448+
return PinState::Excluded;
472449
}
473-
return p;
450+
return PinState::Unspecified;
474451
}
475452

476453
} // namespace OCC

‎src/plugins/vfs/xattr/vfs_xattr.h‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212
#include "common/plugin.h"
1313
#include "common/result.h"
1414

15-
#include "config.h"
16-
1715
namespace xattr {
1816

1917
struct PlaceHolderAttribs {
@@ -40,8 +38,6 @@ struct PlaceHolderAttribs {
4038
namespace OCC {
4139
class HydrationJob;
4240

43-
using namespace xattr;
44-
4541
class VfsXAttr : public Vfs
4642
{
4743
Q_OBJECT
@@ -88,7 +84,7 @@ public Q_SLOTS:
8884

8985
private:
9086
QString xattrOwnerString() const;
91-
PlaceHolderAttribs placeHolderAttributes(const QString& path);
87+
xattr::PlaceHolderAttribs placeHolderAttributes(const QString& path);
9288
OCC::Result<void, QString> addPlaceholderAttribute(const QString &path, const QString &name = {}, const QString &val = {});
9389
OCC::Result<void, QString> removePlaceHolderAttributes(const QString& path);
9490

0 commit comments

Comments
 (0)