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

Support AF_UNIX sockets for mpd #1531

Closed
dondelelcaro opened this Issue Jun 30, 2016 · 10 comments

Comments

5 participants
@dondelelcaro
Contributor

dondelelcaro commented Jun 30, 2016

It would be great if mopidy supported AF_UNIX style sockets for mpd instead of listening on localhost. [This is especially useful for configurations where mopidy is running as a user, and may have a conflict on localhost:6600.]

Something like the following would probably work (but there's probably a better, more general method.)

diff --git a/mopidy/internal/network.py b/mopidy/internal/network.py
index cefdf8e..ae5a87b 100644
--- a/mopidy/internal/network.py
+++ b/mopidy/internal/network.py
@@ -79,9 +79,14 @@ class Server(object):
         self.register_server_socket(self.server_socket.fileno())

     def create_server_socket(self, host, port):
-        sock = create_socket()
-        sock.setblocking(False)
-        sock.bind((host, port))
+        if (re.match(r'/',host):
+            sock = socket.socket(socket.AF_UNIX)
+            sock.setblocking(False)
+            sock.bind(host)
+        else:
+            sock = create_socket()
+            sock.setblocking(False)
+            sock.bind((host, port))
         sock.listen(1)
         return sock
@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 1, 2016

Does this have an advantage over just using another port?

@dondelelcaro

This comment has been minimized.

Contributor

dondelelcaro commented Jul 1, 2016

Yes, because a user can control sockets in directories they own; they have no control over what service gets spawned on a non-privileged port. It also means that only a user who can write to the socket can talk to mopidy, which is often useful.

(For me, it means that I don't have to worry if there's a system-wide mopidy running; my configuration will always "just work" because it uses MPD_HOST=~/var/mpd/socket.)

dondelelcaro added a commit to dondelelcaro/mopidy that referenced this issue Jul 8, 2016

@dondelelcaro

This comment has been minimized.

Contributor

dondelelcaro commented Jul 8, 2016

develop...dondelelcaro:develop seems to work here; someone with better python-fu can probably make this more elegant than what I've done.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 15, 2016

Seems perfectly reasonable to add something like this. Would just need to decide if he expand the Hostname config type to check startswith('/') similar to what you've done or add a socket_path setting separately?

@adamcik adamcik added the A-mpd label Jul 15, 2016

@dondelelcaro

This comment has been minimized.

Contributor

dondelelcaro commented Jul 20, 2016

I think that checking the hostname parameter would be enough; I suggest not just limiting to startswith('/') so you could potentially do hostname=~/var/mpd/socket, but I think that startswith('/') or startswith(~) would be work, or maybe just os.path.expanduser(value).startswith('/')?

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Jun 28, 2017

I'm very keen on this functionality, but am new to mopidy. It looks like nothing has happened from this so far so I'd be willing to give it a go although I'll need to get up to speed on development within this project.
Is it something you would consider accepting if I did do it?

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jun 28, 2017

Yes, certainly, the comments above still stand.

If you have not already seen, there's lots of info to get started at https://docs.mopidy.com/en/latest/contributing/ and https://docs.mopidy.com/en/latest/devenv/. Feel free to ask any questions here, on IRC or at https://discuss.mopidy.com/.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 4, 2017

Can we support this for http also? i.e. https://gist.github.com/superduper/5579037

@jradtilbrook

This comment has been minimized.

Contributor

jradtilbrook commented Jul 4, 2017

Wow hadn't considered that. Seems pretty cool but would you want to wrap that in a separate issue/PR?

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 4, 2017

That's probably a good idea, maybe just worth keeping in mind for your PR that anything reusable would be welcome.

kingosticks added a commit that referenced this issue Feb 13, 2018

Fix/1531 add unix domain socket (#1629)
mpd: add functionality for unix domain socket (Fixes #1531)

The Hostname config type now supports a Unix socket path prefixed with `unix:`

@jodal jodal added this to the v2.2 milestone Mar 30, 2018

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