Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KDESKTOP-1262] Do not force file status after an upload session abortion #366

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ for file in ${staged_files}
do
# Source file filtering is based on file extensions.
if [ -f "${file}" ] && [[ "${file}" =~ ^.*\.(ino|cpp|cc|c|h|hpp|hh|mm)$ ]]; then
clang-format -i ${file} --style="file:${GIT_DIR}/../.clang-format"
clang-format -i ${file} --style="file"
git add ${file}
formatting_exit_code=$?
if [ ${formatting_exit_code} -ne 0 ]; then
Expand Down
14 changes: 8 additions & 6 deletions extensions/MacOSX/kDriveLiteSync/Extension/main.m
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#import "../../../../src/libcommonserver/io/fileAttributes.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the common declarations in vfs.h ?
Or create a new dynamic library "libcommonserverext" with the common code.

#include "xpcService.h"
#include "fileAttributes.h"

#import <Foundation/Foundation.h>
#import <EndpointSecurity/EndpointSecurity.h>
Expand Down Expand Up @@ -109,10 +109,11 @@ static BOOL processAuthOpen(const es_message_t *msg, BOOL *thumbnail)
msg->process->signing_id.data);*/

// Check file status
long bufferLength = getxattr([filePath UTF8String], [EXT_ATTR_STATUS UTF8String], NULL, 0, 0, 0);
long bufferLength = getxattr([filePath UTF8String], [@EXT_ATTR_STATUS UTF8String], NULL, 0, 0, 0);
if (bufferLength >= 0) {
char status[bufferLength];
if (getxattr([filePath UTF8String], [EXT_ATTR_STATUS UTF8String], status, bufferLength, 0, 0) != bufferLength) {
if (getxattr([filePath UTF8String], [@EXT_ATTR_STATUS UTF8String], status, bufferLength, 0, 0)
!= bufferLength) {
NSLog(@"[KD] ERROR: fgetxattr() failed for file %@: %d", filePath, errno);
return FALSE;
}
Expand Down Expand Up @@ -194,14 +195,15 @@ static BOOL processAuthRename(const es_message_t *msg)
}

// Check file status
long bufferLength = getxattr([filePath UTF8String], [EXT_ATTR_STATUS UTF8String], NULL, 0, 0, 0);
long bufferLength = getxattr([filePath UTF8String], [@EXT_ATTR_STATUS UTF8String], NULL, 0, 0, 0);
char status[bufferLength];
if (bufferLength >= 0) {
if (getxattr([filePath UTF8String], [EXT_ATTR_STATUS UTF8String], status, bufferLength, 0, 0) != bufferLength) {
if (getxattr([filePath UTF8String], [@EXT_ATTR_STATUS UTF8String], status, bufferLength, 0, 0)
!= bufferLength) {
NSLog(@"[KD] ERROR: fgetxattr() failed for file %@: %d", filePath, errno);
return FALSE;
}

if (status[0] != EXT_ATTR_STATUS_ONLINE[0]) {
return FALSE;
}
Expand Down
24 changes: 12 additions & 12 deletions extensions/MacOSX/kDriveLiteSync/Extension/xpcService.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,21 @@
}

@property(retain) XPCServiceProxy *_Nullable xpcServiceProxy;
@property(retain) NSMutableDictionary<NSString *, NSMutableSet<NSString *> *> *registeredFoldersMap;
@property(retain) NSMutableDictionary<NSString *, NSMutableSet<NSString *> *> *_Nullable registeredFoldersMap;

- (BOOL)isFileMonitored:(NSString *)filePath;
- (BOOL)isFileMonitored:(NSString *_Nonnull)filePath;
- (void)initOpenWhiteListThumbnailSet;
- (void)initOpenWhiteListSet;
- (void)initOpenBlackListSet;
- (BOOL)isAppInOpenWhiteListThumbnail:(NSString *)appSigningId;
- (BOOL)isAppInOpenWhiteList:(NSString *)appSigningId;
- (BOOL)matchGlob:(NSString *)string globToMatch:(NSString *)glob;
- (BOOL)isAppInOpenBlackList:(NSString *)appSigningId filePath:(NSString *)filePath;
- (BOOL)isAppInDefaultBlackList:(NSString *)appSigningId;
- (void)addAppToFetchList:(NSString *)appSigningId;
- (void)sendMessage:(NSString *)filePath query:(NSString *)verb oneApp:(BOOL)one;
- (BOOL)fetchFile:(NSString *)filePath pid:(pid_t)pid;
- (BOOL)fetchThumbnail:(NSString *)filePath pid:(pid_t)pid;
- (BOOL)isDirectory:(NSString *)path error:(NSError *_Nullable *_Nullable)error;
- (BOOL)isAppInOpenWhiteListThumbnail:(NSString *_Nonnull)appSigningId;
- (BOOL)isAppInOpenWhiteList:(NSString *_Nonnull)appSigningId;
- (BOOL)matchGlob:(NSString *_Nonnull)string globToMatch:(NSString *_Nonnull)glob;
- (BOOL)isAppInOpenBlackList:(NSString *_Nonnull)appSigningId filePath:(NSString *_Nonnull)filePath;
- (BOOL)isAppInDefaultBlackList:(NSString *_Nonnull)appSigningId;
- (void)addAppToFetchList:(NSString *_Nonnull)appSigningId;
- (void)sendMessage:(NSString *_Nullable)filePath query:(NSString *_Nonnull)verb oneApp:(BOOL)one;
- (BOOL)fetchFile:(NSString *_Nonnull)filePath pid:(pid_t)pid;
- (BOOL)fetchThumbnail:(NSString *_Nonnull)filePath pid:(pid_t)pid;
- (BOOL)isDirectory:(NSString *_Nonnull)path error:(NSError *_Nullable *_Nullable)error;

@end
4 changes: 3 additions & 1 deletion src/libcommon/log/customlogwstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
#pragma once

#include <sstream>
#include "libcommon/utility/types.h"

#include <QIODevice>

class CustomLogWStream : private std::wstringstream {
public:
Expand Down
2 changes: 1 addition & 1 deletion src/libcommonserver/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ set(libcommonserver_SRCS
)

if(APPLE)
list(APPEND libcommonserver_SRCS io/iohelper_mac.mm io/iohelper_mac.cpp)
list(APPEND libcommonserver_SRCS io/iohelper_mac.mm io/iohelper_mac.cpp io/fileAttributes.h)
list(APPEND libcommonserver_SRCS utility/utility_mac.mm)
elseif(WIN32)
list(APPEND libcommonserver_SRCS io/iohelper_win.h io/iohelper_win.cpp)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

#pragma once

#define EXT_ATTR_STATUS @"com.infomaniak.drive.desktopclient.litesync.status"
#define EXT_ATTR_PIN_STATE @"com.infomaniak.drive.desktopclient.litesync.pinstate"
#define EXT_ATTR_STATUS "com.infomaniak.drive.desktopclient.litesync.status"
#define EXT_ATTR_PIN_STATE "com.infomaniak.drive.desktopclient.litesync.pinstate"

#define EXT_ATTR_STATUS_ONLINE "O"
#define EXT_ATTR_STATUS_OFFLINE "F"
Expand Down
9 changes: 9 additions & 0 deletions src/libcommonserver/io/iohelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,15 @@ struct IoHelper {
*/
static bool setXAttrValue(const SyncPath &path, const std::string &attrName, const std::string &value,
IoError &ioError) noexcept;

//! Removes the value of the extended attribute with specified name for the item indicated by path.
/*!
\param path is the file system path of the item.
\param attrName is the name of the extended attribute to be removed.
\param ioError holds the error returned when an underlying OS API call fails.
\return true if no unexpected error occurred, false otherwise.
*/
static bool removeXAttr(const SyncPath &path, const std::string &attrName, IoError &ioError) noexcept;
#endif

#ifdef _WIN32
Expand Down
26 changes: 17 additions & 9 deletions src/libcommonserver/io/iohelper_mac.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

#include "libcommon/utility/types.h"

#include "libcommonserver/io/filestat.h"
#include "libcommonserver/io/iohelper.h"
#include "libcommonserver/io/fileAttributes.h"
#include "libcommonserver/log/log.h"
#include "libcommonserver/utility/utility.h"

Expand Down Expand Up @@ -66,20 +66,21 @@ bool IoHelper::getXAttrValue(const SyncPath &path, const std::string &attrName,
isSymlink ? XATTR_NOFOLLOW : 0) != bufferLength) {
ioError = posixError2ioError(errno);
if (ioError == IoError::ResultOutOfRange) {
// XAttr length has changed, retry
continue;
continue; // The extended attribute length has changed, let us retry.
}
return _isXAttrValueExpectedError(ioError);
}

// XAttr has been read
ioError = IoError::Success;
return true;

return true; // The extended attribute `attrName` has been read.
}
}

bool IoHelper::setXAttrValue(const SyncPath &path, const std::string &attrName, const std::string &value,
IoError &ioError) noexcept {
ioError = IoError::Success;

ItemType itemType;
if (!getItemType(path, itemType)) {
LOGW_WARN(logger(), L"Error in IoHelper::getItemType for " << Utility::formatIoError(path, itemType.ioError).c_str());
Expand All @@ -100,17 +101,24 @@ bool IoHelper::setXAttrValue(const SyncPath &path, const std::string &attrName,
return _isXAttrValueExpectedError(ioError);
}

// XAttr has been set
return true; // The extended attribute `attrName` has been set.
}

bool IoHelper::removeXAttr(const SyncPath &path, const std::string &attrName, IoError &ioError) noexcept {
ioError = IoError::Success;
return true;

if (removexattr(path.native().c_str(), attrName.c_str(), XATTR_NOFOLLOW) == -1) {
ioError = posixError2ioError(errno);
return _isXAttrValueExpectedError(ioError);
}

return true; // The extended attribute `attrName` has been removed.
}

bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydrated, IoError &ioError) noexcept {
isDehydrated = false;
ioError = IoError::Success;

static const std::string EXT_ATTR_STATUS = "com.infomaniak.drive.desktopclient.litesync.status";

std::string value;
const bool result = IoHelper::getXAttrValue(itemPath.native(), EXT_ATTR_STATUS, value, ioError);
if (!result) {
Expand Down
1 change: 0 additions & 1 deletion src/libcommonserver/io/iohelper_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -245,5 +245,4 @@ bool isLocked(const SyncPath &path) {
return isLocked;
}


} // namespace KDC
5 changes: 0 additions & 5 deletions src/libcommonserver/utility/utility_mac.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,6 @@ namespace KDC {
bool setFileDates(const SyncPath &filePath, std::optional<KDC::SyncTime> creationDate,
std::optional<KDC::SyncTime> modificationDate, bool symlink, bool &exists);

static bool init_private() {
return true;
}

static void makeMessage() {}

bool moveItemToTrash(const SyncPath &itemPath, std::string &errorStr);
bool preventSleeping(bool enable);
Expand Down
1 change: 1 addition & 0 deletions src/libsyncengine/jobs/abstractjob.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ class AbstractJob : public Poco::Runnable {
}

virtual void abort();
bool hasVfsForceStatusCallback() const noexcept { return _vfsForceStatus != nullptr; };
bool isAborted() const;

[[nodiscard]] inline bool bypassCheck() const { return _bypassCheck; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

#include "jobs/abstractjob.h"
#include "utility/types.h"
#include "db/syncdb.h"
#include "uploadsessionchunkjob.h"
#include "uploadsessionfinishjob.h"
#include "uploadsessionstartjob.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
*/

#include "driveuploadsession.h"
#include "libcommonserver/io/fileAttributes.h"
#include "libcommonserver/io/iohelper.h"
#include "utility/utility.h"

namespace KDC {
Expand All @@ -30,15 +32,15 @@ DriveUploadSession::DriveUploadSession(int driveDbId, std::shared_ptr<SyncDb> sy
DriveUploadSession::DriveUploadSession(int driveDbId, std::shared_ptr<SyncDb> syncDb, const SyncPath &filepath,
const SyncName &filename, const NodeId &remoteParentDirId, SyncTime modtime,
bool liteSyncActivated, uint64_t nbParalleleThread /*= 1*/) :
AbstractUploadSession(filepath, filename, nbParalleleThread), _driveDbId(driveDbId), _syncDb(syncDb), _modtimeIn(modtime),
_remoteParentDirId(remoteParentDirId) {
AbstractUploadSession(filepath, filename, nbParalleleThread),
_driveDbId(driveDbId), _syncDb(syncDb), _modtimeIn(modtime), _remoteParentDirId(remoteParentDirId) {
(void) liteSyncActivated;
_uploadSessionType = UploadSessionType::Drive;
}

DriveUploadSession::~DriveUploadSession() {
if (_vfsForceStatus && !_vfsForceStatus(getFilePath(), false, 100, true)) {
LOGW_WARN(getLogger(), L"Error in vfsForceStatus: " << Utility::formatSyncPath(getFilePath()).c_str());
LOGW_WARN(getLogger(), L"Error in vfsForceStatus: " << Utility::formatSyncPath(getFilePath()));
}
}

Expand Down Expand Up @@ -109,4 +111,27 @@ bool DriveUploadSession::handleCancelJobResult(const std::shared_ptr<UploadSessi

return true;
}

void DriveUploadSession::abort() {
AbstractUploadSession::abort();
setVfsForceStatusCallback(nullptr);

#ifdef __APPLE__
// Removing all extended attributes so that the file that failed to be uploaded is not identified as a dehydrated placeholder
// by the application when restarting the synchronization.

const SyncPath &localPath = getFilePath();
static const std::vector<const char *> extendedAttributes = {EXT_ATTR_STATUS, EXT_ATTR_PIN_STATE};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the new function IoHelper::removeLiteSyncXAttrs


for (const auto attribute: extendedAttributes) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions.

  • Should we remove all attributes or just EXT_ATTR_STATUS? (What is the definition of a placeholder for the application?)
  • Is a missing permission an actual problem for the user?
  • In case of unexpected failure, should the exit code be changed?

Copy link
Contributor

@ClementKunz ClementKunz Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove all attributes or just EXT_ATTR_STATUS? (What is the definition of a placeholder for the application?)

A file is a placeholder if it has the EXT_ATTR_STATUS extended attribute (see LiteSyncExtConnector::vfsGetStatus). So it should be enough to remove just this one. However, there is no point to keep only the pin state.
Moreover, the definition of a placeholder based solely on the status is also arguable. Perhaps basing it on the pinstate instead could help us display the correct status in Finder without risking to allow the deshydration of a not yet synced item.

Copy link
Contributor

@ClementKunz ClementKunz Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a missing permission an actual problem for the user?

Maybe. Perhaps we lose permissions during upload, which caused the abort. So the status has been set but cannot be removed, meaning that the file could be dehydrated while not synced, therefor corrupted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of unexpected failure, should the exit code be changed?

Maybe again ^^ but what can we do if the access rights have been removed 🥲

Comment on lines +124 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in a utility function outside this class. Especially because normalupload (meaning direct uploads, not upload session) are also concerned.

if (auto ioError = IoError::Success;
!IoHelper::removeXAttr(localPath, attribute, ioError) || ioError != IoError::NoSuchFileOrDirectory) {
LOGW_WARN(getLogger(), L"Error in IoHelper::removeXAttr with extended attribute "
<< L"'" << Utility::s2ws(attribute) << L"': "
<< Utility::formatIoError(localPath, ioError));
}
}
#endif
}

} // namespace KDC
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class DriveUploadSession : public AbstractUploadSession {
inline const NodeId &nodeId() const { return _nodeId; }
inline SyncTime modtime() const { return _modtimeOut; }

void abort() override;

protected:
bool handleStartJobResult(const std::shared_ptr<UploadSessionStartJob> &StartJob, std::string uploadToken) override;
bool handleFinishJobResult(const std::shared_ptr<UploadSessionFinishJob> &finishJob) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@
*/

#include "uploadsessionfinishjob.h"
#include "libcommonserver/utility/utility.h"
#include "utility/jsonparserutility.h"
#include "libcommonserver/utility/utility.h"

namespace KDC {

UploadSessionFinishJob::UploadSessionFinishJob(UploadSessionType uploadType, int driveDbId, const SyncPath &filepath,
const std::string &sessionToken, const std::string &totalChunkHash,
uint64_t totalChunks, SyncTime modtime) :
AbstractUploadSessionJob(uploadType, driveDbId, filepath, sessionToken), _totalChunkHash(totalChunkHash),
_totalChunks(totalChunks), _modtimeIn(modtime) {
AbstractUploadSessionJob(uploadType, driveDbId, filepath, sessionToken),
_totalChunkHash(totalChunkHash), _totalChunks(totalChunks), _modtimeIn(modtime) {
_httpMethod = Poco::Net::HTTPRequest::HTTP_POST;
}

Expand All @@ -38,7 +38,7 @@ UploadSessionFinishJob::UploadSessionFinishJob(UploadSessionType uploadType, con
UploadSessionFinishJob::~UploadSessionFinishJob() {
if (_vfsForceStatus) {
if (!_vfsForceStatus(_filePath, false, 0, true)) {
LOGW_WARN(_logger, L"Error in vfsForceStatus for path=" << Path2WStr(_filePath).c_str());
LOGW_WARN(_logger, L"Error in vfsForceStatus for " << Utility::formatSyncPath(_filePath));
}
}
}
Expand Down
Loading
Loading