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

document-portal: Don't invalidate directories #1243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hfiguiere
Copy link
Collaborator

@hfiguiere hfiguiere commented Dec 20, 2023

This work around issue #1234

fuse_lowlevel_notify_inval_entry() will invalidate directories too, which is a problem as it invalidate the current workdir directory. So now we don't invalidate them and will let the kernel deal with it.

This is consistent with the passthrough_hp example from fuse.

@hfiguiere hfiguiere self-assigned this Dec 20, 2023
@hfiguiere hfiguiere force-pushed the issue-1234 branch 2 times, most recently from c7bd171 to 1ba8834 Compare December 25, 2023 17:39
@hfiguiere
Copy link
Collaborator Author

Turns out it prevents the filesystem to be ejected.

So this is unlikely to be the right fix.

@hfiguiere
Copy link
Collaborator Author

Right now the situation is as follow:

I haven't found a different solution that would not hold the directories too long.

This works around issue flatpak#1234. This is a followup from flatpak#1190.

fuse_lowlevel_notify_inval_entry() will invalidate directories too,
which is a problem as it invalidate the current workdir directory.
So now we don't invalidate them and will let the kernel deal with it.

This is consistent with the passthrough_hp example from fuse.

But this still make the directory being held for longer than
necessary.

Signed-off-by: Hubert Figuière <[email protected]>
@hfiguiere hfiguiere marked this pull request as ready for review January 13, 2024 02:24
@hfiguiere
Copy link
Collaborator Author

hfiguiere commented Jan 13, 2024

My take is as follow: after applying this patch the use case that is broken in #1234 is fixed. But this make the directories being held too much as in #689.

I don't know how to solve both properly but I think this patch present a better tradeoff as a work around fix.

@nurupo
Copy link

nurupo commented Jan 16, 2024

Does it even make sense for anything to be using a directory under /run/user/1000/doc/ as its CWD?

(A little rant about applications using `/run/user/1000/doc/`)

While on this topic, something that has been bothering me a lot is that if I save a file in a flatpak Chromium that uses xdg-desktop-portal to a remote drive at /media/my-drive/foobar/, the next time I save a file, the directory that the save dialog window opens up in is something like /run/user/1000/doc/db0809ab/ instead of the previous /media/my-drive/foobar/. While /run/user/1000/doc/db0809ab/ does list my previously saved file, if I save the 2nd file into this directory, the 2nd file doesn't appear in /media/my-drive/foobar/ alongside the 1st file, the 1st file is in there by itself, the 2nd file only appears in /run/user/1000/doc/db0809ab/ alongside the 1st file. So when saving multiple files into the same directory, I have to fight the save file dialog window resetting to /run/user/1000/doc/db0809ab/ and manually navigate to /media/my-drive/foobar/. EVERY. SINGLE. TIME. I save a file. It's so annoying. I feel like Chomium using /run/user/1000/doc/ is some sort of a bug and it shouldn't be used.


@sonnyp
Copy link

sonnyp commented Jan 19, 2024

@nurupo please refer to #475

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

Successfully merging this pull request may close these issues.

3 participants