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

AppInfo cleanups, pidfd based pid mapping, device portal removal #1281

Merged
merged 12 commits into from
Apr 4, 2024

Conversation

swick
Copy link
Collaborator

@swick swick commented Feb 8, 2024

Pulled out from #1268. All the parts that can be merged right now without requiring the new dbus Containers1 interface.

@swick
Copy link
Collaborator Author

swick commented Mar 13, 2024

Ping. Anyone up for a review?

@swick swick force-pushed the wip/pidfd branch 2 times, most recently from 10e6b2e to fcea56b Compare March 14, 2024 15:21
@swick
Copy link
Collaborator Author

swick commented Mar 14, 2024

Unfortunately had to revert a commit from #1279 because it depends on xdp_get_app_info_from_pid. #1280 also uses it, so we need to figure out how to deal with this.

@swick
Copy link
Collaborator Author

swick commented Mar 18, 2024

Rebased onto #1281 which makes the tests work again.

/cc @GeorgesStavracas @smcv

swick added 7 commits April 2, 2024 14:21
The Device portal is, to the best of our knowledge, not used by any
component. It's also weird because it's not a portal expoed to clients
which caused confusion.

The service it provides is also provided by the permission store, minus
the ability to map from arbitrary PID to an app id. This PID to app id
mapping isn't something that can be done in general and is most likely
broken when the PID is not of the xdg-dbus-proxy.

Let's just remove it entirely.
One was a left-over and for the other we just removed the last users.
This will become useful once we support the dbus Containers1 interface
and a new XDP_APP_INFO kind can still refer to a flatpak.
…fo_sync

This will make it easier to follow which method of identiying an app
will be used under what conditions.
Instead of returning either NULL with error, NULL without error and
non-NULL without error, adhere to GLib convention and return either TRUE
with an out param set or FALSE with an error set.
swick added 5 commits April 4, 2024 19:37
The org.fdo.DBus.GetConnectionCredentials method gives us both a PID
(ProcessID) and a pidfd (ProcessFD) in one roundtrip. It fails when the
PID can't be retrieved but allows the pidfd to be -1.

The pidfd will become useful for host and Containers1 clients later.
Every app kind that can somehow get to a pidns of the calling process
should be able to do the pid mappings. This will be used in the next
commit.
Both host apps and snaps connect directly to dbus and thus have a pidfd
that points to the calling process. This allows us to do pid mappings
independent of the app kind.

This will only work when the D-Bus broker supports pidfd (ProcessFD).
@GeorgesStavracas GeorgesStavracas added this pull request to the merge queue Apr 4, 2024
@GeorgesStavracas GeorgesStavracas added this to the 1.20 milestone Apr 4, 2024
Merged via the queue into flatpak:main with commit efd2374 Apr 4, 2024
4 checks passed
@piotrdrag
Copy link
Contributor

You need to update POTFILES.in as well: https://l10n.gnome.org/module/xdg-desktop-portal/#main

@GeorgesStavracas
Copy link
Member

@piotrdrag do you think it's possible to add such check as a CI step to this project? I think it's better if we make POTFILES.in being up-to-date a pre-condition to merging PRs

@piotrdrag
Copy link
Contributor

piotrdrag commented Apr 7, 2024

Various projects in and around GNOME already do that, usually with a custom script or the long-abandoned intltool-update. I don’t know how the former works out, but intltool-update -m is unreliable in many ways.

@swick
Copy link
Collaborator Author

swick commented Apr 8, 2024

Opened #1331 but if anyone has pointers at how to handle this well in CI I might add that as well...

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

Successfully merging this pull request may close these issues.

3 participants