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

Fix RenameOp compatibility with macfuse 4.x #107

Merged
merged 1 commit into from Jan 30, 2022

Conversation

vitalif
Copy link
Contributor

@vitalif vitalif commented Oct 8, 2021

Closed-source macfuse 4.x has broken compatibility with osxfuse 3.x: it passes an additional 64-bit field (flags) after RenameIn regardless that we don't enable the support for RENAME_SWAP/RENAME_EXCL.

The simplest fix is just to check for the presence of all-zero flags and strip them when they're present.

// closed-source macfuse 4.x has broken compatibility with osxfuse 3.x:
// it passes an additional 64-bit field (flags) after RenameIn regardless
// that we don't enable the support for RENAME_SWAP/RENAME_EXCL
// the simplest fix is just to check for the presence of all-zero flags
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be much harder to enable support for RENAME_SWAP/RENAME_EXCL?
The current approach seems pretty hackish…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sorry for long absence. There are some quirks - as I understand macfuse kernel module doesn't indicate the format in any way if you don't request these features in the init function yourself (the author did a bad thing), and I'm not sure what happens if you do request it with older osxfuse versions - maybe they'll decide that you want a missing feature and fail.

So it would require some testing with different versions of macos and I'm not a huge closed-source systems fan, even I had a macbook the first thing that I'd do would be to remove macos and install Linux :)

These features are mac-specific, Linux and POSIX in general doesn't have them. So... I don't know. I don't want to spend a lot of time testing different macos versions currently, and I think this change is anyway good for your library because it at least fixes normal rename support. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, sorry for long absence. There are some quirks - as I understand macfuse kernel module doesn't indicate the format in any way if you don't request these features in the init function yourself (the author did a bad thing), and I'm not sure what happens if you do request it with older osxfuse versions - maybe they'll decide that you want a missing feature and fail.

Ugh, that sounds annoying. But maybe it could be fixed on the osxfuse? Have you tried reporting it to the developers?

So... I don't know. I don't want to spend a lot of time testing different macos versions currently, and I think this change is anyway good for your library because it at least fixes normal rename support. :-)

I don’t have time to maintain this library for macOS, so unfortunately I can’t just accept code contributions where the author (you, in this case) doesn’t have time to maintain or change/verify/debug the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-) something like that. This change definitely works as it's simple and it's included in https://github.com/yandex-cloud/geesefs builds and tested by colleagues at Yandex many of whom are mac users )). In fact the bug was initially found by the author of our another internally used fuse-based FS (with a C++ binding). I tried reporting the problem but he came over to that bug and said that he doesn't want them to break compatibility the second time ))). So, yeah. Pretty annoying.))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I don't like the idea of rename with additional flags/modes at all. POSIX doesn't have it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you insist on adding the support for rename_swap/rename_excl? If you say so I can try to do that at some point. Until then renames will just remain broken under macfuse :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the context. Can you expand the comment stating that you tried reporting this to osxfuse upstream but they are unwilling to revert the change?

Also, can you rebase your changes to the latest master branch contents and ensure you’re formatting the file with gofmt please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, updated the branch

Closed-source macfuse 4.x has broken compatibility with osxfuse 3.x:
it passes an additional 64-bit field (flags) after RenameIn regardless
that we don't enable the support for RENAME_SWAP/RENAME_EXCL.

The simplest fix is just to check for the presence of all-zero flags
and strip them when they're present.
@stapelberg stapelberg merged commit 108387e into jacobsa:master Jan 30, 2022
@stapelberg
Copy link
Collaborator

Thanks!

@vitalif
Copy link
Contributor Author

vitalif commented Jan 30, 2022

Thank you for merging, I'll get to the other PR soon too :-)

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.

None yet

2 participants