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

✨ Implement unix:dir and unix:tmpdir for --address #175

Merged
merged 3 commits into from
Dec 7, 2024

Conversation

jokeyrhyme
Copy link
Contributor

No description provided.

Copy link
Collaborator

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

src/bus/mod.rs Outdated Show resolved Hide resolved
src/bus/mod.rs Outdated Show resolved Hide resolved
According to https://dbus.freedesktop.org/doc/dbus-specification.html ,
for unix:tmpdir, the service _may_ create an abstract socket,
but is not required to do so, thus our implementation is the same as our
unix:dir, as suggested by the specification
@jokeyrhyme jokeyrhyme force-pushed the implement-unix-dir-and-tmpdir-sockets branch from e08433c to 8d67580 Compare December 4, 2024 10:36
Copy link
Collaborator

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM. It's not being tested though. If you update the bus::default_address function to use a unix dir address (should be trivial) that would make sense anyway but also mean that this path is tested.

For UNIX systems, the default is now $XDG_RUNTIME_DIR/dbus-$RANDOM
@jokeyrhyme
Copy link
Contributor Author

I've changed default_address() and can confirm that cargo run causes a file like "/run/user/60222/dbus-3418628410" to exist on my system

Copy link
Collaborator

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Nice! LGTM but I've one question as I wrote on the matrix channel:

I'm a bit curious though. The session bus config on my system does indeed have only one listen element set to listen>unix:tmpdir=/tmp however, the session bus is actually listening on /run/user/1000/bus

I wonder if that's because dbus-broker (the broker on my Fedora) is simply ignoring the listen directive in the config and defaulting to a hardcoded path for session. If so, I'd really want to follow their example. This would mean that I asked you to add the last commit for no good reason but in my defence, I didn't know about this discrepancy.

@zeenix zeenix merged commit 1e8a567 into dbus2:main Dec 7, 2024
8 checks passed
@jokeyrhyme jokeyrhyme deleted the implement-unix-dir-and-tmpdir-sockets branch December 7, 2024 22:05
@zeenix
Copy link
Collaborator

zeenix commented Dec 12, 2024

I wonder if that's because dbus-broker (the broker on my Fedora) is simply ignoring the listen directive in the config and defaulting to a hardcoded path for session. If so, I'd really want to follow their example. This would mean that I asked you to add the last commit for no good reason but in my defence, I didn't know about this discrepancy.

Seems like when combined with #159, we end up still using the tmpdir from the config:

❯ cargo r -- --print-address
unix:tmpdir=/tmp,guid=3686ef2f3b7dd8236e7fc55c675b0474

We've 2 options:

  1. Go the dbus-broker route completely on this and that means reverting this PR and ignoring the listen elements from the config in parse XML configuration files #159.
  2. Forget about what dbus-broker is doing and re-add the removed commit to use tmpdir when no config is available/found.

Given that I can't really figure out what exactly dbus-broker is doing here (I searched though their issues but didn't find anything relevant), I'd vote for taking the simpler route of option 2 for now.

Moreover, the address being printed is not connectable and is server-side only address so this option can't be used like it's supposed to be. I'm not sure how you're testing the changes but the method I've been using to test changes, is to run DBUS_SESSION_BUS_ADDRESS=<address from busd> cargo t in zbus repo. Testing with any client would have shown this issue btw. 😉 We really need more tests.

Anyway, if you could fix these two issues in a new PR, that would be great.

@jokeyrhyme
Copy link
Contributor Author

Ah, I wasn't using the --print-address at all, I was going directly to where I expected the socket file to be

@zeenix
Copy link
Collaborator

zeenix commented Dec 12, 2024

Ah, I wasn't using the --print-address at all, I was going directly to where I expected the socket file to be

Ah ok. I guess you were looking for the random socket file in the dir specified?

@jokeyrhyme
Copy link
Contributor Author

Yeah, and that works
I just neglected to holistically test that along with the pre-existing functionality
I should be able to take a look at this over the weekend

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.

2 participants