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

Backwards compatibility silently broken around 4.x #839

Closed
vitalif opened this issue Sep 30, 2021 · 14 comments
Closed

Backwards compatibility silently broken around 4.x #839

vitalif opened this issue Sep 30, 2021 · 14 comments
Assignees
Labels
Milestone

Comments

@vitalif
Copy link

vitalif commented Sep 30, 2021

Hi
In these commits:
osxfuse/fuse@f95e939
osxfuse/fuse@3ec24b8
You added a field into the Rename operation format. Now the kernel by default sends rename requests that are incompatible with FS servers that don't support the newest ABI, including older versions of your own library. This is bad because it breaks backwards compatibility and all 3rdparty bindings, for example, Go bindings.
It will be much better if you check for a flag passed by the FS server and enable the new ABI only when the client library passes the new flag.
Can't submit a PR because macfuse is now closed source :-E hehe. :-)

@bfleischer
Copy link
Member

The kernel extension only sends the additional flags when the user space server sets FUSE_CAP_RENAME_SWAP or FUSE_RENAME_EXCL. At least that's how it is supposed to work. How does this break 3rd party bindings?

@vitalif
Copy link
Author

vitalif commented Sep 30, 2021

Hm. Can these flags conflict with something else? For example with an older flag value? I updated @jacobsa/fuse Go binding to work with macfuse 4.x (it didn't work because it used the old mounting method without COMMFD) and the app started to receive renames in the new format...

@vitalif
Copy link
Author

vitalif commented Sep 30, 2021

The kernel extension only sends the additional flags when the user space server sets FUSE_CAP_RENAME_SWAP or FUSE_RENAME_EXCL. At least that's how it is supposed to work. How does this break 3rd party bindings?

As I understand it's not the userspace server that sets these flags, I receive it in initOp.Flags and I send 1<<5 | 1<<22 back i.e. InitBigWrites | InitMaxPages. See https://github.com/vitalif/fusego/blob/macfuse-commfd/connection.go#L160

After that I get "new" renames.

I.e. from my point of view it seems that the kernel sends these flags to userspace and doesn't check if userspace actually supports them.

The binding code is here: https://github.com/vitalif/fusego/tree/macfuse-commfd

@bfleischer
Copy link
Member

I.e. from my point of view it seems that the kernel sends these flags to userspace and doesn't check if userspace actually supports them.

The kernel sends all supported flags to user space:

fiii->flags = FUSE_ATOMIC_O_TRUNC | FUSE_ALLOCATE | FUSE_RENAME_SWAP | FUSE_RENAME_EXCL | FUSE_CASE_INSENSITIVE | FUSE_VOL_RENAME | FUSE_XTIMES;

#if M_OSXFUSE_ENABLE_EXCHANGE
fiii->flags |= FUSE_EXCHANGE_DATA;
#endif

The FUSE server running in user space then needs to respond with the flags that it supports. If you don't send FUSE_RENAME_SWAP and FUSE_RENAME_EXCL back, the kernel will not send new rename messages to user space.

@bfleischer
Copy link
Member

bfleischer commented Sep 30, 2021

How do you know that you receive "new" renames?

@bfleischer
Copy link
Member

bfleischer commented Sep 30, 2021

Oh, I think I know what you mean. The offset of the file names is not the same as before (FUSE for macOS 3) in case the user space server uses ABI version 7.19, but does not support FUSE_RENAME_SWAP or FUSE_RENAME_EXCL.

@bfleischer bfleischer added this to the 4.2.1 milestone Sep 30, 2021
@kikht
Copy link

kikht commented Sep 30, 2021

If you will decide to change something here, please consider that some applications have already adopted this change. E.g. they might implement something similar to https://github.com/osxfuse/fuse/blob/master/lib/fuse_lowlevel.c#L1299-L1305 Current state of affairs makes it possible to write code that works correctly across 3.x/4.x versions by checking against flags provided in INIT message. Simply reverting struct size for everyone who does not request FUSE_CAP_RENAME_SWAP/FUSE_CAP_RENAME_EXCL during initialization will break this.

@bfleischer
Copy link
Member

bfleischer commented Sep 30, 2021

We would need to increment the FUSE ABI version to address this properly. But this won't be possible because the next ABI version already exists for FUSE on Linux and comes with new feature requirements that macFUSE does not support right now. So incrementing the ABI version is not an option.

The libfuse version that ships with macFUSE 4 supports the old 7.19 ABI without renamex support and the new 7.19 ABI with renamex support. But the new macFUSE 4 kernel extension will always send the old file name with an offset of 16 bytes instead of 8 bytes (old 7.19 ABI).

@bfleischer
Copy link
Member

Current state of affairs makes it possible to write code that works correctly across 3.x/4.x versions by checking against flags provided in INIT message.

That was the intention of the flags. There really is no pretty solution to the problem without being able to increment the ABI version number.

@bfleischer
Copy link
Member

The offset change is transparent to file systems linking against libfuse. All third party FUSE libraries needed or need to be updated anyway to call mount_macfuse (instead of the old mount_osxfuse) or to open the new /dev/macfuse devices directly (instead of the old /dev/osxfuse devices).

This means that the ABI modification is "technically" not breaking backwards compatibility, since older versions of third party FUSE libraries do not work with macFUSE 4 without being updated. That's why I opted to modify the ABI in the way I did. It's not ideal, but incrementing the ABI version was not an option.

@bfleischer
Copy link
Member

bfleischer commented Sep 30, 2021

@kikht I agree, modifying the ABI now would do more harm than good. The update from version 3 to version 4 introduced several breaking changes for third party FUSE libraries. However, macFUSE 4 is binary compatible with legacy file systems that were built using version 3 (linked against libosxfuse.2.dylib or OSXFUSE.framework).

@vitalif I'm going to close this issue. I don't think there is anything I can do to address this without breaking backward compatibility, which would require a major version change.

@vitalif
Copy link
Author

vitalif commented Sep 30, 2021

How do you know that you receive "new" renames?

2021/09/30 16:19:28.038607 fuse.DEBUG Op 0x0000000b        connection.go:411] <- Rename (PID 91644, old_parent 1, old_name "", new_parent 1, new_name "\x00\x00\x00\x00\x00\x00\x00newname_testfile\x00testfile")

@vitalif
Copy link
Author

vitalif commented Sep 30, 2021

However, macFUSE 4 is binary compatible with legacy file systems that were built using version 3 (linked against libosxfuse.2.dylib or OSXFUSE.framework).

How is that possible if old versions also don't know about the offset? :) or do they? :)

@bfleischer
Copy link
Member

libfuse is a shared library and knows about the new offset. Old apps are linking against libfuse. This means that the app (or file system implementation, to be more specific) does not need to know about the offset change.

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

No branches or pull requests

3 participants