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

xdg-document-portal: implement flock #1353

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

Conversation

lufia
Copy link

@lufia lufia commented Apr 29, 2024

I implemented flock operation of FUSE.

This is needed for Steam client.

Tested

I tested whether the new xdg-document-portal accepts flock(2) syscall with flock(1).

$ umount /run/user/60331/doc
$ meson compile -C _build
$ ./_build/document-portal/xdg-document-portal &

$ cd /run/user/60331/doc
$ touch lock.L
$ flock -n lock.L sleep 100

Then, within 100 seconds, I ran flock(1) in other terminal session.

$ flock -n lock.L ls || echo BAD
BAD

I checked blocking operations are rejected.

$ flock lock.L ls
flock: lock.L: Function not implemented

$ flock -s lock.L ls
flock: lock.L: Function not implemented

@lufia lufia marked this pull request as ready for review April 29, 2024 08:43
@swick
Copy link
Contributor

swick commented Apr 29, 2024

Looks like this will block the entire document portal when any file is blocked due to flock.

@lufia
Copy link
Author

lufia commented Apr 29, 2024

Hi, @swick
I mitigated not to block entire document-portal by rejecting blocking operations of flock.
Steam client works well in my Arch Linux box.

@hfiguiere Thanks for your suggestion!

@GeorgesStavracas
Copy link
Member

Thanks for the patch. Please squash the suggestion commit into a single commit.

@swick
Copy link
Contributor

swick commented Apr 29, 2024

Supporting flock only for certain option is very suspicious to me. This needs review from someone who knows about fuse and filesystems.

This is needed for Steam client

Signed-off-by: Kyohei KADOTA <[email protected]>
Co-authored-by: Hubert Figuière <[email protected]>
@Dutt-A
Copy link

Dutt-A commented Jul 23, 2024

Hello,
I'm wondering if someone can again take a look at this branch and merge it in. Without this addition, the Steam flatpak is unable to handle adding another filesystem.

@swick
Copy link
Contributor

swick commented Jul 23, 2024

@alexlarsson ^

@smcv
Copy link
Collaborator

smcv commented Jul 29, 2024

Without this addition, the Steam flatpak is unable to handle adding another filesystem

You can give the unofficial Steam Flatpak app direct access to locations where you want to add an extra Steam library by completely exiting from Steam, and then using something like:

$ mkdir /media/other-disk/steam-library
$ flatpak override --user --filesystem=/media/other-disk/steam-library com.valvesoftware.Steam

before starting Steam again and selecting /media/other-disk/steam-library as your new Steam library. This avoids the performance overhead of going through a FUSE filesystem, as well as avoiding any limitations like the one fixed in this branch.

(documentation)

@Dutt-A
Copy link

Dutt-A commented Jul 29, 2024

Without this addition, the Steam flatpak is unable to handle adding another filesystem

You can give the unofficial Steam Flatpak app direct access to locations where you want to add an extra Steam library by completely exiting from Steam, and then using something like:

$ mkdir /media/other-disk/steam-library
$ flatpak override --user --filesystem=/media/other-disk/steam-library com.valvesoftware.Steam

before starting Steam again and selecting /media/other-disk/steam-library as your new Steam library. This avoids the performance overhead of going through a FUSE filesystem, as well as avoiding any limitations like the one fixed in this branch.

(documentation)

Upon using the override I precisely get the original errorno 38 (function not implemented) that lufia mentions here, and I believe that the additions are need to allow the line to work, unless I am misunderstanding something.

@chrisawi
Copy link

Upon using the override I precisely get the original errorno 38 (function not implemented) that lufia mentions here, and I believe that the additions are need to allow the line to work, unless I am misunderstanding something.

No, the override is sufficient by itself. You do need to restart Steam, and you may need to remove and re-add the library in Steam so it doesn't continue to use the portal path. That error means it's probably still going through the document portal.

Also, just to be clear, you need to modify the path to match your actual Steam library location; don't use literally /media/other-disk/steam-library.

@Dutt-A
Copy link

Dutt-A commented Jul 29, 2024

Upon using the override I precisely get the original errorno 38 (function not implemented) that lufia mentions here, and I believe that the additions are need to allow the line to work, unless I am misunderstanding something.

No, the override is sufficient by itself. You do need to restart Steam, and you may need to remove and re-add the library in Steam so it doesn't continue to use the portal path. That error means it's probably still going through the document portal.

Also, just to be clear, you need to modify the path to match your actual Steam library location; don't use literally /media/other-disk/steam-library.

Really? I'll take a look and try again in a few days (occupied with moving right now).

I actually recall that the override was working for me perfectly fine on the Fedora KDE spin, but somehow it has not been working for me on the Fedora Sway spin. Both installations should have me running on Wayland.

I appreciate the input.

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

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

I think this is the least worst compromise for the moment.

@swick
Copy link
Contributor

swick commented Oct 21, 2024

Do you understand the implications of supporting a weird subset of flags for flock?

flock does something on a per-process basis but any flock call on fuse will result in flock being called in the docuement portal process. Do we know what the results are of this?

@jadahl
Copy link
Collaborator

jadahl commented Oct 21, 2024

Do we know what the results are of this?

I suspect the risk is some app does flock(file) will cause other apps that flock() the same file to not actually receive said lock, meaning the lock is useless.

There also seems to be missing "cleanup" of files left locked by some app, meaning an app doing flock() and then crashes before flock(LOCK_UN) will mean the lock will remain indefinitely.

@GeorgesStavracas
Copy link
Member

Do you understand the implications of supporting a weird subset of flags for flock?

The implication is that xdg-document-portal won't support flock() with a particular subset of flags.

I also want to ask you to please stop calling things "weird". It doesn't set a productive tone to this conversation.

flock does something on a per-process basis but any flock call on fuse will result in flock being called in the docuement portal process. Do we know what the results are of this?

I do not know. If you do, please inform us.

I'll go ahead and merge this, as a stop-gap solution. It'll be interesting to be able to support blocking flock() calls somehow, maybe by spawning a thread for each blocking call? @lufia are you interested in exploring this next step?

@GeorgesStavracas
Copy link
Member

Eh, nevermind, @jadahl raised a good point about app cleanup...

@jadahl
Copy link
Collaborator

jadahl commented Oct 21, 2024

I guess one could implement file locking on behalf of document portal owners in the document portal, i.e. as long as there is more than one lock owner, we call flock(), and then handle LOCK_SH vs LOCK_EX on a per app basis. But it does mean apps will be able to "lock" files it got shared, for better or worse. I guess as long as they already have write access, it isn't that bad.

@swick
Copy link
Contributor

swick commented Oct 21, 2024

Sorry, but I just don't think it makes a lot of sense to merge things that we don't fully understand even if they seem important to some apps. I'd equally appreciate if you'd stop assuming that everything I say is an attack on you. Calling it weird that just the nonblocking flock variants are supported isn't far fetched because AFAICS when nonblock is supported, so is the blocking variant. What we have here definitely isn't something that apps are used to seeing which can easily introduce issues in some interesting code paths.

@Mikenux
Copy link

Mikenux commented Oct 23, 2024

@lufia

  • Is the game library correctly detected and added in Steam (while making sure that no static permission was added to add the library, i.e. no flatpak override ...)?
  • Are the games playable?

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.

9 participants