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

lib/fs,model: Clarify errors for Android filenames (fixes #9395) #9397

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tomasz1986
Copy link
Contributor

lib/fs,model: Clarify errors for Android filenames (fixes #9395)

Currently, Syncthing tries to sync files to Android devices with no
consideration for its filename restrictions (see [1]). This commit makes
it so that similarly to Windows, Syncthing relies on a hard-coded list
of characters which are invalid on Android and displays a specific error
message when user tries to sync a file with one of the characters in its
name.

[1] https://stackoverflow.com/a/64021421

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

)

Currently, Syncthing tries to sync files to Android devices with no
consideration for its filename restrictions (see [1]). This commit makes
it so that similarly to Windows, Syncthing relies on a hard-coded list
of characters which are invalid on Android and displays a specific error
message when user tries to sync a file with one of the characters in its
name.

[1] https://stackoverflow.com/a/64021421

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
// even if the filesystem is not FAT. On Windows, they apply to both FAT
// and NTFS.
// Ref: https://cs.android.com/android/platform/superproject/main/+/main:packages/providers/MediaProvider/src/com/android/providers/media/util/FileUtils.java;drc=9d054a9eb6a8d8c3d79e007ea6d2f89c94a52f06;l=485
const fatDisallowedCharacters = (`/\<>:"|?*` +
Copy link
Contributor

Choose a reason for hiding this comment

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

androidDisallowedCharacters?

Copy link
Contributor Author

@tomasz1986 tomasz1986 Feb 6, 2024

Choose a reason for hiding this comment

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

Isn't fat better in this case? It's more generic and that's essentially where these characters come from (even though they are applied on a wider scale than just on FAT).

Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine someone reading the code later on thinking "what the hell, you can't blindly assume that the user has a fat filesystem. *reads comment* Ah, that's why".

@rasa
Copy link
Member

rasa commented Feb 6, 2024

So, Linux/Mac/BSD users on exFAT filesystems, will still see the generic message, yes?

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Feb 6, 2024

So, Linux/Mac/BSD users on exFAT filesystems, will still see the generic message, yes?

Yeah, but on those you would need to actually detect the filesystem, while here both Android and Windows are a special case, where the restrictions apply regardless of the filesystem.

Just for information, I've changed the PR to a draft, as I need to figure out how to fix the failing tests.

@tomasz1986 tomasz1986 marked this pull request as draft February 6, 2024 22:10
@JanKanis
Copy link

JanKanis commented Apr 25, 2024

This change will globally disallow FAT reserved characters on Android. But not all of Android is fat. On my phone, primary internal storage is backed by an ext4 fs, and reserved characters can be used just fine. AFAIK that is not an uncommon arrangement on phones with enough internal storage.

The stackoverflow reference also doesn't say fat reserved characters are forbidden on android, they are only forbidden on fat-backed filesystems.

Syncthing should be able to just work on ext4-backed storage. There is a bug somewhere preventing that now, but that is a separate issue.

A solution based on the filesystem in question would be much better. That could also cover unixes using a fat filesystem.

@bt90
Copy link
Contributor

bt90 commented Apr 26, 2024

I remember discussing this with @tomasz1986 and coming to the conclusion that the overlay filesystem on newer versions of Android blocks this, regardless of whether it's based on ext4 or not.

@JanKanis
Copy link

Hmm, that would suck when I buy a new phone. But still, fat reserved characters are working (more or less) on my phone right now, and it looks like this change will overzealously block fat reserved characters for me when merged. If I read that right in the code, this change would be a regression for my usage of syncthing if it is not paired with a configuration option or some kind of encoding option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add error message when filename contains illegal characters for target storage
4 participants