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

Enable TCP connections and export createClient method #23

Closed
wants to merge 2 commits into from
Closed

Enable TCP connections and export createClient method #23

wants to merge 2 commits into from

Conversation

martenjacobs
Copy link
Contributor

I'd like to connect to a bus on a remote machine (for easier development from my mac). To do this, I've disabled authentication on my linux VM's system bus and started up socat to forward my dbus to the correct socket.
To be able to connect to the TCP endpoint, I exported the createClient method in de index.js module and added support for TCP connections. I've tested this to be working in my set-up:

let remoteSystemBus = dbus.createClient({
  busAddress: "tcp:host=10.211.55.36,port=7272",
  authMethods: ["ANONYMOUS"]
});
let obj = await remoteSystemBus.getProxyObject(
  "org.freedesktop.NetworkManager",
  "/org/freedesktop/NetworkManager"
);
let props = obj.getInterface("org.freedesktop.DBus.Properties");
let result = await props.Get("org.freedesktop.NetworkManager", "Version");
console.log(result);
// Variant {signature: "s", value: "1.6.2"}

@acrisci
Copy link
Member

acrisci commented May 2, 2019

I wonder if this is testable.

* interfaces or request service names. Connects to the socket specified by the
* properties assigned.
*/
module.exports.createClient = createClient;
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to expose this. Just use sessionBus(). It's the same thing

https://github.com/acrisci/node-dbus-next/blob/a241269c6e917e22ae6e9f1015713751192469ff/index.js#L40-L42

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but that's a bit of a strange way to access the system bus...

Copy link
Member

Choose a reason for hiding this comment

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

You can allow to override the address for systemBus() if you want. Or propose to have only one function to create a bus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or propose to have only one function to create a bus.

Isn't this actually what I'm doing here, other than leaving two 'convenience' functions for systemBus and sessionBus? Removing those would be quite a major breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

quite a major breaking change.

FYI I'm still probably about 2 months from a stable api and have some other major breaking changes planned that are worse than this. It's something I want to get right because I'm copying this pattern in the python version of this library.

I think one or two functions would be a good idea, but three is too many.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about something like this?

dbus.connect({
  bus: "system" | "session" | "unix:path=/tmp/foo" | "tcp:host=localhost,port=1234"
})

Copy link
Member

Choose a reason for hiding this comment

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

I like the way this looks actually. Open a new issue for it though because there are some subtle points.

@@ -32,7 +32,9 @@ function createStream(opts) {
try {
switch (family.toLowerCase()) {
case 'tcp':
throw new Error('tcp dbus connections are not supported');
host = params.host || 'localhost';
port = params.port;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I like the idea of being able to override address string properties with option parameters because then we have to do it for every parameter. Just specify it in the address like tcp:host=127.0.0.1,port=4242 and we should be able to parse it out.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, that's actually what this is doing, my mistake. I mistook params for opts. 👍

@acrisci
Copy link
Member

acrisci commented May 2, 2019

You shouldn't be required to pass the auth type as well. The protocol has a way of negotiating what is supported between the client and server and negotiating something that works.

@martenjacobs
Copy link
Contributor Author

You shouldn't be required to pass the auth type as well. The protocol has a way of negotiating what is supported between the client and server and negotiating something that works.

I know, I chose to limit to ANONYMOUS because there's a issue with this line:
https://github.com/acrisci/node-dbus-next/blob/4283e0e535cf6ca519e8d839bc9216ec30b45830/lib/handshake.js#L21

On my set-up, getUserHome() returns undefined (this may be due to mine running in the Electron renderer process). This causes path.join to fail (this happens in the upstream repo as well). When I limit to ANONYMOUS that auth method is not attempted so the error is bypassed (this is fine for me because this connection is just for dev). I can add this as a separate issue if you'd like

@acrisci
Copy link
Member

acrisci commented May 2, 2019

I can add this as a separate issue if you'd like

Yeah go ahead.

@martenjacobs
Copy link
Contributor Author

Superseded by #25 and #26

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.

None yet

2 participants