Avoid panic if card gives bad state

[HEnquist]

I got a bug report on my DSP project, where it panics when reading the state of a pcm. The panic comes from here : https://github.com/diwic/alsa-rs/blob/master/src/pcm.rs#L159 I'm guessing the card returns an invalid value for the state, but it doesn't happen with any of the hardware I have so I can't check that. Anyway, would it be an option to add an "Invalid" or "Unknown" state variant to avoid a panic with misbehaving hardware? Or let state() return a Result? I can implement something and make a PR, but let's agree on the ifs and how's first :)

[diwic]

Hi!

So a new/unknown state seems to be a fairly severe bug in the alsa library (or kernel) and I think panicking is the only reasonable thing to do. (I just checked and there does not seem to be any new states implemented in git master of alsa-lib.)

But this bug needs to be reported and fixed. I think we should do it like this:

• Expose a function pub fn state_raw(&self) -> c_int.
• In your code, you can copy-paste State::from_c_int and change it as you see fit, to make it convert a c_int to a Result<State, c_int>.

Use this information to report bugs upstream to alsa. Let me know if you have problems with this part; I have long experience with working with ALSA upstream.

[HEnquist]

This approach makes sense. Thanks for helping out! I'll prepare a test version of my project and ask the guy who reported the problem to run it. I'll also ask what kernel version etc he is using.

[HEnquist]

Digging a bit more while waiting for the guy to come back with more info. As I understand, the snd_pcm_state() function can also return negative errors. Apparently it's not uncommon that it returns broken pipe error for example. The documentation doesn't mention this for state(), but for the closely related status() it says so. I think this is a much more likely cause for the panic than some new/unkown state. Considering that, wouldn't it make sense to let the PCM::state() use the acheck! macro and return a Result instead?

[diwic]

Digging a bit more while waiting for the guy to come back with more info. As I understand, the snd_pcm_state() function can also return negative errors. Apparently it's not uncommon that it returns broken pipe error for example. The documentation doesn't mention this for state(), but for the closely related status() it says so. I think this is a much more likely cause for the panic than some new/unkown state. Considering that, wouldn't it make sense to let the PCM::state() use the acheck! macro and return a Result instead?

This is not my understanding of snd_pcm_state. Xruns, disconnected devices etc have their specific states and should not cause a negative error code to be returned from snd_pcm_state.

For reporting sound card & system information, see https://www.alsa-project.org/wiki/AlsaInfo

[HEnquist]

Still waiting for the guy to come back with more info, without that it's difficult to get anywhere.

I continued looking at the state() and errors, and I think you are right. All mentions I find of negative results from snd_pcm_state seem to be quite old, so they were probably from bugs that have been fixed.

I had another thought for an improvement that can be implemented right away. Could the panic get a more helpful message? Currently the panic says just "panicked at 'called Result::unwrap() on an Err value: Error("snd_pcm_state", UnsupportedOperation)'". This isn't saying anything about what the problem is. https://github.com/diwic/alsa-rs/blob/master/src/lib.rs#L44 https://github.com/diwic/alsa-rs/blob/master/src/error.rs#L56 Something along the lines of "snd_pcm_state returned an invalid value NNN" would be much more clear.

[diwic]

Something along the lines of "snd_pcm_state returned an invalid value NNN" would be much more clear.

Agreed - unfortunately, this would require a rework of the error handling code. Hence the state_raw function which would enable you to quickly build something just to troubleshoot the alsa library instead.

[HEnquist]

The state_raw function solves my debugging problem nicely, I don't really need anything else. Except for the guy to get back to me that is...

For the better panic message, how about something like this? This doesn't need any changes to the error handling.

    pub fn state(&self) -> State { 
        let rawstate = self.state_raw();
        if let Ok(state) = State::from_c_int(rawstate, "snd_pcm_state") {
            state
        }
        else {
            panic!("snd_pcm_state returned an invalid value of {}", rawstate);
        }
    }

    /// Only used internally, and for debugging the alsa library. Please use the "state" function instead.
    pub fn state_raw(&self) -> c_int { unsafe { alsa::snd_pcm_state(self.0) as c_int } }

[diwic]

@HEnquist I'm okay with the changes you post above - if you go ahead and submit that as a PR I'll merge it.

[HEnquist]

Thanks! I think we can close this now since there isn't any problem in this lib.

[HEnquist]

Just a quick followup. To try to avoid this kind of confusion in the future, I made a small PR to alsa-lib: https://github.com/alsa-project/alsa-lib/pull/162

[diwic]

@HEnquist FYI, I'm not sure the maintainers look at Github PRs. The standard way of submitting patches is to send emails to alsa-devel, Takashi Iwai and Jaroslav Kysela (see email adresses here https://www.kernel.org/doc/html/v5.8/process/maintainers.html ), preferrably using git send-email. If you get no responses, it could be that you submitted it at the wrong place.

[HEnquist]

Yes it's a bit of an experiment. They first mention that the preferred way is via email, and then underneath it says "Patches are also accepted as GitHub pull requests". I'll give it a week before I try again via email.