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.

Sign up for GitHub

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

Jump to bottom

✨ zb: Add support for unixexec transport #976

Merged
merged 6 commits into from
Oct 22, 2024
Merged

✨ zb: Add support for unixexec transport #976

merged 6 commits into from
Oct 22, 2024

Conversation

vially
Copy link
Contributor

@vially vially commented Sep 5, 2024 •
edited
Loading

Add initial support for the unixexec transport (#325). Currently this is only implemented when the tokio feature is enabled. Edit: Support for the async-io runtime has also been implemented in later revisions.

zeenix
zeenix requested changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@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.

Thanks so much for this contribution. 👍 Please see inline comments. :)

zbus/src/address/transport/mod.rs Show resolved Hide resolved
zbus/src/address/transport/mod.rs Outdated Show resolved Hide resolved
zbus/src/address/transport/mod.rs Outdated Show resolved Hide resolved
zbus/src/address/transport/unixexec.rs Outdated Show resolved Hide resolved
zbus/src/address/transport/unixexec.rs Outdated Show resolved Hide resolved
@zeenix zeenix marked this pull request as ready for review September 9, 2024 15:20
@zeenix
Copy link
Contributor

zeenix commented Sep 9, 2024

I took the liberty of declaring the PR "ready for review". I hope you don't mind.

@vially vially changed the title Add (tokio) support for unixexec transport Add support for unixexec transport Sep 9, 2024
@vially vially changed the title Add support for unixexec transport ✨ zb: Add support for unixexec transport Sep 9, 2024
@vially vially force-pushed the unixexec branch 3 times, most recently from a041e3e to d3ac6d3 Compare September 10, 2024 20:57
zeenix
zeenix requested changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@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.

Starting to really get in a great shape. Mostly some nitpicks about git hygiene and naming etc. :)

zbus/src/abstractions/mod.rs Show resolved Hide resolved
zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
zbus/src/connection/socket/unixexec.rs Outdated Show resolved Hide resolved
zbus/src/connection/socket/unixexec.rs Outdated Show resolved Hide resolved
zbus/src/connection/socket/unixexec.rs Outdated Show resolved Hide resolved
zeenix
zeenix requested changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@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.

Other than that, getting very close to me merged. While you rework, if could you please also add a bit of context to each commit message in the details section, that'd be perfect?

zbus/src/connection/socket/command.rs Outdated Show resolved Hide resolved
zbus/src/connection/socket/command.rs Outdated Show resolved Hide resolved
zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
zeenix
zeenix requested changes Sep 14, 2024
View reviewed changes
Copy link
Contributor

@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.

Ok, looks like just one last thing. :)

zbus/src/connection/socket/mod.rs Show resolved Hide resolved
@zeenix
Copy link
Contributor

zeenix commented Sep 15, 2024

@vially I'm terribly sorry but I overlooked something important in my previous reviews: tests. Any chance you can add a testcase for unixexec? Otherwise, there is a good chance I (or someone else) will break the support. It will be good to see that your PR actually works e2e.

This was referenced Sep 15, 2024
Merged
Closed
@vially vially force-pushed the unixexec branch 2 times, most recently from 4c50fff to 52376c8 Compare September 23, 2024 18:55
@vially vially force-pushed the unixexec branch 2 times, most recently from 59bf019 to faee7b1 Compare October 1, 2024 18:25
zeenix
zeenix requested changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@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. Thanks for your patience here!

BTW, "🔌" would be a good emoji for the internal socket-type addition commits. :)

zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
zeenix
zeenix requested changes Oct 2, 2024
View reviewed changes
zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
zbus/src/abstractions/process.rs Outdated Show resolved Hide resolved
@zeenix zeenix mentioned this pull request Oct 4, 2024
Open
@vially vially force-pushed the unixexec branch 2 times, most recently from b19971c to 37273d0 Compare October 11, 2024 19:48
zeenix
zeenix requested changes Oct 13, 2024
View reviewed changes
Copy link
Contributor

@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.

zbus/src/address/transport/unixexec.rs Outdated Show resolved Hide resolved
@zeenix
Copy link
Contributor

zeenix commented Oct 14, 2024

@vially Thank you so much for your extreme patience and hard work on this. However, I've to ask for one last thing: I had to revert the new address API as I believe there was one fundamental flaw in its design that I didn't catch in time (#1068).

I understand if you'd not have the time and energy to rebase your work yet again. If so, please let me know and I'll rebase for you soon.

@vially
Copy link
Contributor Author

vially commented Oct 17, 2024

Sorry for the late response on this, got a bit caught up with other work. I'm planning to rebase this over the weekend but if you want to merge it sooner feel free to rebase it anytime. It's fine by me either way.

vially added 4 commits October 20, 2024 20:26
@vially
✏️ zb: Fix typo in transport module comment
b02f508
@vially
♻️ zb: Enable process abstraction for all unix targets
5b02921 
The `process` abstraction will be used (on `unix` targets)
for spawning child processes when implementing the
`unixexec` D-Bus transport.
@vially
♻️ zb: Add thin wrapper over async Command types
6b4ebc3 
Most of the newly introduced `Command` methods are currently
unused, but they will be needed when implementing the `unixexec`
transport.
@vially
🔌 zb: Add Command stdio socket type
f4e82da 
This socket communicates with a spawned child process via its standard input
and output streams. The main use-case of this socket is for
communicating with programs such as `systemd-stdio-bridge`[0] which
implement a proxy between the `stdin`/`stdout` and a D-Bus bus.

[0](https://www.freedesktop.org/software/systemd/man/latest/systemd-stdio-bridge.html)
zeenix
zeenix requested changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@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. Thanks for your extreme perseverance with this. 🙏

zbus/src/address/transport/unixexec.rs Outdated Show resolved Hide resolved
@zeenix
Copy link
Contributor

zeenix commented Oct 22, 2024

Don't worry about fuzzing failing btw, it's just some issue in the gvariant code.

vially added 2 commits October 22, 2024 17:45
@vially
✨ zb: Add support for unixexec transport
cc05e6e 
The `unixexec` transport communicates with a spawned process via its
standard input and output streams. This transport can be used with
out-of-process forwarder programs such as `systemd-stdio-bridge` as the
basis for the D-Bus protocol.

Links:
- https://dbus.freedesktop.org/doc/dbus-specification.html#transports-exec
- https://www.freedesktop.org/software/systemd/man/latest/systemd-stdio-bridge.html
@vially
✅ zb: Add unixexec connection test case
0fd431b
@zeenix zeenix enabled auto-merge October 22, 2024 19:19
@zeenix zeenix merged commit 155d144 into dbus2:main Oct 22, 2024
7 checks passed
@vially vially deleted the unixexec branch October 22, 2024 19:38
@vially
Copy link
Contributor Author

vially commented Oct 22, 2024

🎉 @zeenix Thanks a lot for your support, patience and guidance in getting this merged. It took a bit more work than I originally anticipated but it has come a long way since my initial hacky approach of piping the data through a bunch of extra sockets.

@zeenix
Copy link
Contributor

zeenix commented Oct 22, 2024

🎉 @zeenix Thanks a lot for your support, patience and guidance in getting this merged.

No! Thank you for your patience and your will to get the work done with the best quality. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elmarco elmarco elmarco left review comments

@zeenix zeenix zeenix approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vially @zeenix @elmarco