Rustify recent changes according to @plietar

[michaelherger]

Somewhat uniform coding style might help myself to better understand Rust :-)

[sashahilton00]

ok, also do you want to remove the -z option while you're at it for the zeroconf-port?

[sashahilton00]

Thanks, just need to check the new behaviour when trying to hook to a port thats <1024 for the zeroconf option, then will merge.

[sashahilton00]

The error when trying to bind to a priviledged port when run as non-root is:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 13, message: "Permission denied" } })', src/libcore/result.rs:906:4

this is a little too cryptic, we need to handle the error and panic with a proper message.

[ComlOnline]

Please correct me if I'm wrong, but this shouldn't be volume should it?

[sashahilton00]

nope, it shouldn't. Probably a copypasta. Also, we need to handle the error when a non-root user tries to set a priviledged port (<1024). Currently it panics with a cryptic error, we need to display a proper error, or default to port 0.

[ComlOnline]

I would be more inclined to say it should through an error (that a human can understand) and quit, as more that likely if you're changing this port its for a reason. However talking of panics like you said elsewhere I don't think a volume out of range is panic worthy, just set as default and show a debug message.

[sashahilton00]

I think we need to make a decision now with regards to how we approach problems like this in future. Do we want librespot to be user friendly, or require precision? If the latter, we should just fail hard on any bad input, or we accomodate bad inputs and throw warnings. Either way, we need to be consistent...

[ComlOnline]

As we're planning a daemon for the future that is going to be user friendly, it can be responsible for potentially fixing inputs. Librespot on its own should require precision as it is after all a library.
Thoughts?

[sashahilton00]

I agree. So it should panic on initial-volume as paul suggested, and also on priviledged ports when not run as root.

[michaelherger]

I think I'd second the opinion that if we're going to have a daemon, then we can stay cryptic in librespot.

[sashahilton00]

If we are going for the precision approach, initial-volume and zeroconf-port should both panic with bad values, the latter when trying to use a priviledged port as a non-root user.

[plietar]

There's no reason to catch ports under 1024 at this point in time.

• librespot maybe running as root. This is not unreasonable on embedded devices or in Docker containers
• In some circumstances it is possible to bind a low port even without being root (process has CAP_NET_BIND_SERVICE for example, bind(2) is being trapped using ptrace and the port number replaced (this is crazy but technically possible), ...)
• Binding may fail even with a high numbered port (port already taken) and you can't predict that, so you have to be ready to deal with errors at that point.

[sashahilton00]

All valid reasons for not defaulting to anything if one tries to set a port under 1024, but surely we should at least panic so that it doesn't just look like a bug? I note that it crashes here: https://github.com/librespot-org/librespot/blob/master/src/discovery.rs#L218, shouldn't we have a handler for the error that just panics with 'Unable to bind port' or similar? Currently there is nothing when it errors to point the user to their use of --zeroconf-port being the issue...

[ComlOnline]

I see no reason that it shouldn't panic and give an informative error at least then said user will hopefully not create an issue.

Missing comment.

[sashahilton00]

Given that we're going for the panic on bad initial-volume and when ports are already taken/unusable, please add an error handler to

librespot/src/discovery.rs

Line 218 in 53fab4d

http.serve_addr_handle(&format!("0.0.0.0:{}", port).parse().unwrap(), &handle, move || Ok(discovery.clone())).unwrap()

with a generic message like "Unable to bind to port" for when it panics, as the standard rust panic is not informative. cc @michaelherger

[michaelherger]

Haha... if only I knew how to do this. I guess that'll be the next Rust lesson for me.

[kingosticks]

Would this just be a case of replacing unwrap() with expect("some informative error message")?

[michaelherger]

Hmm... I applied @kingosticks' suggestion, pushed to my repository. But the change wouldn't show up here?

[kingosticks]

I think I had the same issue and put it down to changing my origin to the new one. I think github gets confused.

[sashahilton00]

If it's easier, you could open a new PR

[michaelherger]

Wow... this repository no longer shows up in the list of repositories I could use as a base fork to submit a pull request! Somehow my fork has lost any link to this repository...

[othiman]

Maybe the reason is that librespot-org was "unforked" from the plietar repo by Github to enable searching.

[michaelherger]

I'm closing this PR, as I had to fix my repository setup due to the "unforking".