Page MenuHome
Differential D11466 Diff 37965 source/blender/blenlib/intern/storage_apple.mm

Changeset View

Standalone View

View Options

source/blender/blenlib/intern/storage_apple.mm

Show All 18 Lines
/** \file /** \file
* \ingroup bli * \ingroup bli
* *
* macOS specific implementations for storage.c. * macOS specific implementations for storage.c.
*/ */
#import <Foundation/Foundation.h> #import <Foundation/Foundation.h>
#include <string>
#include <sys/xattr.h>
#include "BLI_fileops.h" #include "BLI_fileops.h"
#include "BLI_path_util.h" #include "BLI_path_util.h"
/* Extended file attribute used by OneDrive to mark placeholder files. */
static const char *ONEDRIVE_RECALLONOPEN_ATTRIBUTE = "com.microsoft.OneDrive.RecallOnOpen";
/** /**
* \param r_targetpath: Buffer for the target path an alias points to. * \param r_targetpath: Buffer for the target path an alias points to.
* \return Whether the file at the input path is an alias. * \return Whether the file at the input path is an alias.
*/ */
/* False alarm by clang-tidy: #getFileSystemRepresentation changes the return value argument. */ /* False alarm by clang-tidy: #getFileSystemRepresentation changes the return value argument. */
/* NOLINTNEXTLINE: readability-non-const-parameter. */ /* NOLINTNEXTLINE: readability-non-const-parameter. */
bool BLI_file_alias_target(const char *filepath, char r_targetpath[FILE_MAXDIR]) bool BLI_file_alias_target(const char *filepath, char r_targetpath[FILE_MAXDIR])
{ {
Show All 18 Lines if (isSame) {
[targetURL getFileSystemRepresentation:r_targetpath maxLength:FILE_MAXDIR]; [targetURL getFileSystemRepresentation:r_targetpath maxLength:FILE_MAXDIR];
return false; return false;
} }
/* Note that the if-condition may also change the value of `r_targetpath`. */ /* Note that the if-condition may also change the value of `r_targetpath`. */
if (![targetURL getFileSystemRepresentation:r_targetpath maxLength:FILE_MAXDIR]) { if (![targetURL getFileSystemRepresentation:r_targetpath maxLength:FILE_MAXDIR]) {
return false; return false;
} }
} }
ankitmUnsubmitted
Done
Inline Actions

See the blender::StringRef::find* functions.

ankitm: See the `blender::StringRef::find*` functions.
return true; return true;
} }
/**
* Checks if the given string of listxattr() attributes contains a specific attribute.
*
* \param attributes: a string of null-terminated listxattr() attributes.
* \param search_attribute: the attribute to search for.
* \return 'true' when the attribute is found, otherwise 'false'.
*/
static bool find_attribute(const std::string &attributes, const char *search_attribute)
{
/* Attributes is a list of consecutive null-terminated strings. */
const char *end = attributes.data() + attributes.size();
for (const char *item = attributes.data(); item < end; item += strlen(item) + 1) {
if (STREQ(item, search_attribute)) {
return true;
}
}
brechtUnsubmitted
Done
Inline Actions

The "\0"s at the start seems wrong. From the listxattr man page it does not seem like it starts with a null terminator, so the first attribute would not be matched.

brecht: The `"\0"s` at the start seems wrong. From the `listxattr` man page it does not seem like it…
lzandmanAuthorUnsubmitted
Done
Inline Actions

The "\0"s at the start seems wrong. From the listxattr man page it does not seem like it starts with a null terminator, so the first attribute would not be matched.

The first attribute should be handled by if-statement above. By now I've done a dozen implementations of this function. The first one, which was C only, worked pretty well. But @Ankit Meel (ankitm) preferred a std::string solution and we've gone back and forth over a proper implementation. I hope this one suffices.

lzandman: > The `"\0"s` at the start seems wrong. From the `listxattr` man page it does not seem like it…
return false;
}
/**
* Checks if the file is merely a placeholder for a OneDrive file that hasn't yet been downloaded.
*
* \param path: the path of the file.
* \return 'true' when the file is a OneDrive placeholder, otherwise 'false'.
*/
static bool oneDrive_checkIfFileIsPlaceholder(const char *path)
{
ankitmUnsubmitted
Done
Inline Actions

Use ssize_t and check for error (-1).
https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/listxattr.2.html

Also print corresponding errno to error message in that case. https://en.cppreference.com/w/cpp/string/byte/strerror

Also see https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Variable_Scope

ankitm: Use `ssize_t` and check for error (-1). https://developer.apple.
lzandmanAuthorUnsubmitted
Done
Inline Actions

and check for error (-1).
Also print corresponding errno to error message in that case. https://en.cppreference.com/w/cpp/string/byte/strerror

I did, but I didn't know what to do with the error. I opted for returning false, which is kind of weird. But you're saying I should print the error to the console? I didn't know that made sense for Blender. What should happen to the flow? I'm used to higher level programming languages that throw exceptions. Should I just print the error and be maintain my original flow?

lzandman: > and check for error (-1). > Also print corresponding errno to error message in that case.
ankitmUnsubmitted
Done
Inline Actions

size_t is unsigned. So in case of error, its value would be a large positive number instead of -1. That case will not be caught by size < 1. Thus ssize_t should be used and it is what is returned by listxattr also.

Returning false is fine. A debug statement to the console for the error is what I am suggesting:
"Listing attributes failed: no such file or directory: /Users/t/a/b.jpeg"

ankitm: `size_t` is unsigned. So in case of error, its value would be a large positive number instead…
/* Note: Currently only checking for the "com.microsoft.OneDrive.RecallOnOpen" extended file
* attribute. In theory this attribute can also be set on files that aren't located inside a
* OneDrive folder. Maybe additional checks are required? */
/* Get extended file attributes */
ssize_t size = listxattr(path, nullptr, 0, XATTR_NOFOLLOW);
if (size < 1) {
return false;
}
ankitmUnsubmitted
Done
Inline Actions

Is std::string a better choice here ? It has methods to resize storage, cleans up after itself and gives access to underlying buffer by .data().
Later on, searching for substrings is easy with some extra care for the \0. https://godbolt.org/z/xf5xz3KnP

ankitm: Is `std::string` a better choice here ? It has methods to resize storage, cleans up after…
lzandmanAuthorUnsubmitted
Done
Inline Actions

Funny, because I started out using NSString type. But I figured since most of Blender seems to consist of low-level C-code these "fancy" types should be avoided. I will look into it.

lzandman: Funny, because I started out using NSString type. But I figured since most of Blender seems to…
ankitmUnsubmitted
Done
Inline Actions

That might have been an obj-C file, not an obj-C++ file. C++ containers/ STL (and their replacements like blender::Map are preferred over low level fiddling).

ankitm: That might have been an obj-C file, not an obj-C++ file. C++ containers/ STL (and their…
lzandmanAuthorUnsubmitted
Done
Inline Actions

I've used the std::string find() method and threw away my custom code.

lzandman: I've used the std::string find() method and threw away my custom code.
std::string attributes(size, '\0');
brechtUnsubmitted
Done
Inline Actions

Don't print any errors to the console, this message is not useful to users as something they can fix, it will just be an annoyance.

brecht: Don't print any errors to the console, this message is not useful to users as something they…
lzandmanAuthorUnsubmitted
Done
Inline Actions

Getting conflicting review remarks, as @Ankit Meel (ankitm) asked me to include the error printing :-) I will remove it.

lzandman: Getting conflicting review remarks, as @ankitm asked me to include the error printing :-) I…
size = listxattr(path, attributes.data(), size, XATTR_NOFOLLOW);
/* In case listxattr() has failed the second time it's called. */
lzandmanAuthorUnsubmitted
Done
Inline Actions

It just occurred to me that in the switch to using std::string find() method I've introduced a bug. Semantically we are no longer comparing each individual attribute, but doing a substring search on the complete list of attributes as a string. This produces wrong results when the attribute searched for isn't included itself, but a different attribute that happens to start with the same characters is present.

So this result of listxattr() would generate a positive result, even though it doesn't contain the attribute:

"com.microsoft.OneDrive.RecallOnOpen.SomethingElse\0com.microsoft.OneDrive.RecallOnOpen.NothingElseMatters\0"

Guess I'll update the code. Or revert back to my "dumb", but perfect code :-)

lzandman: It just occurred to me that in the switch to using `std::string find()` method I've introduced…
if (size < 1) {
return false;
}
/* Check for presence of 'com.microsoft.OneDrive.RecallOnOpen' attribute. */
return find_attribute(attributes, ONEDRIVE_RECALLONOPEN_ATTRIBUTE);
}
/**
ankitmUnsubmitted
Done
Inline Actions

Can be a global variable in case others are added.

ankitm: Can be a global variable in case others are added.
lzandmanAuthorUnsubmitted
Done
Inline Actions

As in: global to this file? Or should it be globally defined in some other (header) file?

lzandman: As in: global to this file? Or should it be globally defined in some other (header) file?
ankitmUnsubmitted
Done
Inline Actions

this file

ankitm: this file
* Checks if the file is marked as offline and not immediately available.
ankitmUnsubmitted
Done
Inline Actions

const

ankitm: `const`
*
* \param path: the path of the file.
* \return 'true' when the file is a placeholder, otherwise 'false'.
*/
static bool checkIfFileIsOffline(const char *path)
{
/* Logic for additional cloud storage providers could be added in the future. */
return oneDrive_checkIfFileIsPlaceholder(path);
}
eFileAttributes BLI_file_attributes(const char *path) eFileAttributes BLI_file_attributes(const char *path)
{ {
int ret = 0; int ret = 0;
/* clang-format off */ /* clang-format off */
ankitmUnsubmitted
Done
Inline Actions

Comment style (// vs /*) should be uniform in a file. Also remove the ellipses.

ankitm: Comment style (// vs /*) should be uniform in a file. Also remove the ellipses.
@autoreleasepool { @autoreleasepool {
/* clang-format on */ /* clang-format on */
const NSURL *fileURL = [[NSURL alloc] initFileURLWithFileSystemRepresentation:path const NSURL *fileURL = [[NSURL alloc] initFileURLWithFileSystemRepresentation:path
isDirectory:NO isDirectory:NO
relativeToURL:nil]; relativeToURL:nil];
NSArray *resourceKeys =
/* Querying NSURLIsReadableKey and NSURLIsWritableKey keys for OneDrive placeholder files
* triggers their unwanted download. */
NSArray *resourceKeys = nullptr;
const bool is_offline = checkIfFileIsOffline(path);
if (is_offline) {
resourceKeys = @[ NSURLIsAliasFileKey, NSURLIsHiddenKey ];
}
else {
resourceKeys =
@[ NSURLIsAliasFileKey, NSURLIsHiddenKey, NSURLIsReadableKey, NSURLIsWritableKey ]; @[ NSURLIsAliasFileKey, NSURLIsHiddenKey, NSURLIsReadableKey, NSURLIsWritableKey ];
}
ankitmUnsubmitted
Done
Inline Actions

Shorter, drive service agnostic comment would be fine too.

Querying NSURLIsReadableKey and NSURLIsWritableKey keys for placeholder files in cloud services triggers their download.

ankitm: Shorter, drive service agnostic comment would be fine too. Querying `NSURLIsReadableKey` and…
lzandmanAuthorUnsubmitted
Done
Inline Actions

But currently I only noticed this issue for OneDrive placeholder files. iCloud Drive doesn't seem affected by this. And I haven't tested other cloud storage services. I'm in favor of a shorter comment, but I do think it should be OneDrive-specific for now.

lzandman: But currently I only noticed this issue for OneDrive placeholder files. iCloud Drive doesn't…
const NSDictionary *resourceKeyValues = [fileURL resourceValuesForKeys:resourceKeys error:nil]; const NSDictionary *resourceKeyValues = [fileURL resourceValuesForKeys:resourceKeys error:nil];
ankitmUnsubmitted
Done
Inline Actions

const bool ...

ankitm: `const bool ...`
const bool is_alias = [resourceKeyValues[(void)(@"@%"), NSURLIsAliasFileKey] boolValue]; const bool is_alias = [resourceKeyValues[(void)(@"@%"), NSURLIsAliasFileKey] boolValue];
const bool is_hidden = [resourceKeyValues[(void)(@"@%"), NSURLIsHiddenKey] boolValue]; const bool is_hidden = [resourceKeyValues[(void)(@"@%"), NSURLIsHiddenKey] boolValue];
const bool is_readable = [resourceKeyValues[(void)(@"@%"), NSURLIsReadableKey] boolValue]; const bool is_readable = is_offline ||
ankitmUnsubmitted
Done
Inline Actions

What are the values of is_readable and is_writable in the case of a placeholder file ?

ankitm: What are the values of `is_readable` and `is_writable` in the case of a placeholder file ?
lzandmanAuthorUnsubmitted
Done
Inline Actions

I wondered what to do with those. They are false for placeholder files, as no corresponding attribute is found in the collection (because I didn't query for them). The result is the file is marked FILE_ATTR_SYSTEM which does seem kind of weird, but it seems to work out. I think currently the only consequence is that the file is displayed using ICON_SYSTEM (line 1240 in source/blender/editors/space_file/filelist.c).

lzandman: I wondered what to do with those. They are `false` for placeholder files, as no corresponding…
const bool is_writable = [resourceKeyValues[(void)(@"@%"), NSURLIsWritableKey] boolValue]; [resourceKeyValues[(void)(@"@%"), NSURLIsReadableKey] boolValue];
const bool is_writable = is_offline ||
[resourceKeyValues[(void)(@"@%"), NSURLIsWritableKey] boolValue];
if (is_alias) { if (is_alias) {
ret |= FILE_ATTR_ALIAS; ret |= FILE_ATTR_ALIAS;
} }
if (is_hidden) { if (is_hidden) {
ret |= FILE_ATTR_HIDDEN; ret |= FILE_ATTR_HIDDEN;
} }
if (is_readable && !is_writable) { if (is_readable && !is_writable) {
ret |= FILE_ATTR_READONLY; ret |= FILE_ATTR_READONLY;
} }
if (!is_readable) { if (!is_readable) {
ret |= FILE_ATTR_SYSTEM; ret |= FILE_ATTR_SYSTEM;
} }
if (is_offline) {
ret |= FILE_ATTR_OFFLINE;
}
} }
return (eFileAttributes)ret; return (eFileAttributes)ret;
} }