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

WIP: Update to zbus git #142

Closed
wants to merge 1 commit into from
Closed

WIP: Update to zbus git #142

wants to merge 1 commit into from

Conversation

elmarco
Copy link
Contributor

@elmarco elmarco commented Oct 2, 2024

There is an issue with a non-Send future..

There is an issue with non-Send future
@@ -36,7 +34,7 @@ pub struct Bus {
// All (cheaply) cloneable fields of `Bus` go here.
#[derive(Clone, Debug)]
pub struct Inner {
address: Address,
address: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not acceptable to me. We should move from generic strings to specific types, not the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but this is going to be refactored next when I rebase the config, to support multiple listenable address (same as dbus-daemon, apparently the last is printed in --print-address).

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm.. I'm not sure I want that. We should only support 1 address at a time. I want to keep things simpler. Unless there is a compelling use case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, I have some use cases in mind, like allowing the guest services to connect to a host VM bus (think qemu / spice agents etc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds good at first but giving raw direct access to the host's system bus would be a bad idea, security wise. Even for session, it can be argued that it's not good. There is a reason why xdg-dbus-proxy exists since people don't even want apps running on the same host, unfiltered access to the bus. So I wonder if there could better solutions for this, especially ones that don't require listening to multiple connections.

Also to keep in mind that you wouldn't want users to have to modify the dbus configuration but rather make everything work out of the box.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, it is a private bus

That's cool, but then why does it need to listen on multiple addresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the guest services can connect to the host bus, and management apps can use regular unix socket.

Copy link
Collaborator

@zeenix zeenix Oct 3, 2024

Choose a reason for hiding this comment

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

so the guest services can connect to the host bus

You mean through vsock? I am not sure why qemu does it differently but the Firecracker's way of using a plain unix socket on the host side for vsock, makes a lot of sense to me. That will also solve this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need some kind of vsock<->unix bridge for that, and you loose direct peer details.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need some kind of vsock<->unix bridge for that, and you loose direct peer details.

like peer credentials? I thought that's not possible and why we use anonymous auth with vsock. 🤔

@zeenix
Copy link
Collaborator

zeenix commented Oct 2, 2024

There is an issue with a non-Send future..

Fixed your regression for you: dbus2/zbus#1036 :)

@elmarco
Copy link
Contributor Author

elmarco commented Oct 2, 2024

There is an issue with a non-Send future..

Fixed your regression for you: dbus2/zbus#1036 :)

weird stuff... why does it need to be declared Send+Sync+'static?? why isn't it infered like the rest ...

@zeenix
Copy link
Collaborator

zeenix commented Oct 3, 2024

There is an issue with a non-Send future..

Fixed your regression for you: dbus2/zbus#1036 :)

weird stuff... why does it need to be declared Send+Sync+'static?? why isn't it infered like the rest ...

Because you're the one declaring the type explicitly and therefore get to decided the guarantees. I know it's painful and annoying..

@zeenix
Copy link
Collaborator

zeenix commented Oct 7, 2024

I rebased this branch on main here. It builds but all the tests fail/hang. I haven't investigated what the issue is but pretty sure it's some minor thing in either the branch or regression in zbus.

@zeenix
Copy link
Collaborator

zeenix commented Oct 7, 2024

I rebased this branch on main here. It builds but all the tests fail/hang. I haven't investigated what the issue is but pretty sure it's some minor thing in either the branch or regression in zbus.

I investigated this and it is indeed a regression in zbus main. dbus2/zbus#1055.

@zeenix
Copy link
Collaborator

zeenix commented Oct 7, 2024

I rebased this branch on main here. It builds but all the tests fail/hang. I haven't investigated what the issue is but pretty sure it's some minor thing in either the branch or regression in zbus.

I investigated this and it is indeed a regression in zbus main. dbus2/zbus#1055.

This has been fixed in zbus git main, and my branch passes the tests. However, I have not addressed the review comments here, just made it build and pass the CI locally. I'd suggest resetting this branch to mine before addressing the review comments.

@zeenix
Copy link
Collaborator

zeenix commented Oct 14, 2024

Closing in favor of #151.

@zeenix zeenix closed this Oct 14, 2024
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