mirror of
https://github.com/librespot-org/librespot.git
synced 2026-04-27 00:05:55 +03:00
[GH-ISSUE #486] mDNS binds to all interfaces #306
Labels
No labels
A-Alsa
SpotifyAPI
Tokio 1.0
audio
bug
can't reproduce
compilation
dependencies
duplicate
enhancement
good first issue
help wanted
high priority
imported
imported
invalid
new api
pull-request
question
reverse engineering
wiki
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/librespot#306
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @frafall on GitHub (May 31, 2020).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/486
Originally assigned to: @Johannesd3 on GitHub.
I run librespot on a Raspian which also runs a series of docker containers, which implies a shitload of networking interfaces, not all of them routable on the local network. BUT, the mdns responder in librespot includes all of these in the published record. This results in inconsistent connectivity/detection for Spotify clients.
Just to verify I built a macvlan setup with a separate network namespace including the macvlan interface only which resulted in the expected:
Now, I assume this is an oversight but we should have a config/commandline option to specify which network interface to bind to. As discovery is a core function in the librespot library we probably need to include it here and not push it to "future daemon".
For now I get around this with a network namespace limiting the visibility of interfaces for the daemon but this is not quite a well-known nor documented configuration nor should we expect every implementor such as spotifyd/raspotify/vollibrespot to known and implement.
Best regards
Frafall
Example of local interfaces when running on a host also running docker containers:
Macvlan config:
@ashthespy commented on GitHub (May 31, 2020):
Currently
libmdnsdoesn't let us select an interface to listen on, and binds to all non loopback ones.There is an issue upstream to enhance this, but needs work..
@frafall commented on GitHub (May 31, 2020):
Well, got Vollibrespot running in a network namespace with macvlan, publishes only what I want and get reliable discovery, only needed to make sure ipv6 and lo interface was properly set up.
But, I'd guess more ppl than me get unreliable discovery due to docker messing up network interfaces.
Nice work on the Vollibrespot btw, using it due to metadata and control functions :)
@ashthespy commented on GitHub (May 31, 2020):
Interesting workaround, like you said probably helpful in other situations as well!
I should really clean it up, it started of as a quick PoC for Volumo, but nice to see that it's useful to others as well! :-)
@roderickvd commented on GitHub (Aug 7, 2021):
@Johannesd3 I thought about assigning this to you, as you have new mDNS in the works.
@schnabel commented on GitHub (Jan 3, 2022):
I have the same problem and reported upstream.
@willstott101 commented on GitHub (May 8, 2022):
https://github.com/librespot-org/libmdns/pull/35 released as 0.7
@NePoCz commented on GitHub (Sep 16, 2022):
Hi, since librespot now uses libmdns 0.7 which added support for binding to interface(s), would it be possible to implement this option?
I would try a PR myself, but unfortunately have no coding experience so stepping asside here
@JasonLG1979 commented on GitHub (Sep 16, 2022):
Sure, if @roderickvd thinks it's a good idea I think I can do that pretty easily.
@JasonLG1979 commented on GitHub (Sep 16, 2022):
It would probably be something like
discovery-interfacesthat would be a comma separated list or a single interface, that defaults to "any" or "all" (I'll have to look at libmdns to see what our options are?).@roderickvd commented on GitHub (Sep 16, 2022):
Sure why not? The "why not" of course is not to have too many command line options, but in this case I think it's in fair demand and a new option is warranted.
@setime commented on GitHub (Oct 31, 2022):
I have a proposal for implementing an option to specify to which interfaces the mDNS responder shall bind. I am a new to Rust so my code will be for sure not optimal. I am unsure how to implement parsing IPv4 and IPv6 addresses. I used the libmdns exmaple from here as a reference.
Please give feedback :).
@ashthespy commented on GitHub (Oct 31, 2022):
You shouldn't need
regexto go from string toIpAddr, you can use the convenientFromStrtrait, so could use do something like this :-)@setime commented on GitHub (Oct 31, 2022):
Thanks for the hint. Avoid a regex seems like a good idea. The main issue I had with the propose approach is that I am unable to recover if the string cannot be parsed, e.g.
192.168.0.a. This ends in a panic. I think it would be better to print a corresponding message and then bind to all IP addresses, so it doesn't fail.I tried to resolve this but I was not able to. There is no pattern like
try & catchin Rust, which I probably would have used in python or so. I tried to look for something to replace it but I was unable to getassert()orparse(),expect()andunwrap()working. Does anybody has a good idea here?Anyhow removing the regex yields much cleaner code:
@roderickvd commented on GitHub (Oct 31, 2022):
You should look into
IpAddr::from_str. The Rust idiom for success or failure is returning and doing something with aResult. Welcome to Rust 👍@ashthespy commented on GitHub (Oct 31, 2022):
And and
filter_mapandmap_errare your friends when playing with lists :-)@setime commented on GitHub (Nov 1, 2022):
Alright thanks for the hints. I will read up and try again ;)
@setime commented on GitHub (Nov 2, 2022):
I was able to write something up with
filter_mapandmap_erras well asunwrap_or_else(). Looking at the rest of the codeunwrap_or_else()seems the way to go and then exiting the program. Hence I would propose:Continuing in case of an error:
@ashthespy commented on GitHub (Nov 2, 2022):
Looks good, please feel free to open a PR! :-)
There are some minor things you could tweak, e.g
Splitis an iterator already, so you don't need tocollect() -> into_iter()it.. You couldbind_ip.split(",").map(....)but the rest looks goodI hope you're enjoying Rust :-)
@setime commented on GitHub (Nov 5, 2022):
I have created a PR.
Thanks for the help and advice. So far I like Rust.
@ashthespy commented on GitHub (Nov 25, 2022):
Implemented with #1071, @setime please update the wiki and we can close this! :-)
@setime commented on GitHub (Nov 25, 2022):
I updated the wiki :).