[pulseaudio-discuss] [PATCH] module-mmkbd-evdev: Don't crash on failure to close fd

[Alexander E. Patrakov]

12.09.2014 14:10, David Henningsson wrote:
> If the keyboard is unplugged, it looks like the kernel is reporting
> back -ENODEV when trying to close the fd. This is probably a kernel
> error, but still, it's better to complain than to crash.
>
> Buglink: https://bugs.freedesktop.org/show_bug.cgi?id=80867
> Reported-by: Stelios Bounanos
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
>   src/modules/module-mmkbd-evdev.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)

The patch neither looks definitely good nor looks definitely bad to me 
(but hey, it fixes a crash!). I have verified that there are no other 
instances of pa_close in the same module (i.e. that could attempt to 
close the same device and thus would need a similar quirk), but am not 
sure whether the "log the close() failure" logic belongs here or should 
be done generally in pa_close().

I have also looked at the definition of pa_close(). While it is 
unrelated to the problem that the patch is trying to solve, I must say 
that either any pa_assert_se(pa_close(fd) == 0) in any file is a bug, or 
the loop that retries on EINTR inside pa_close() is a bug, or both. The 
retry-loop is a bug at least for Linux. Here is why.

https://lwn.net/Articles/576478/
https://lwn.net/Articles/576591/

Even for the hypothetical situation where close() can fail with EINTR, 
see the manual page:

> In particular close() should not be retried after an EINTR since this 
> may  cause  a  reused descriptor from another thread to be closed.

But it is retried. The worst case is already described above, and the 
best case is that it, when retried, fails with EBADFD and causes the 
assert to fail.


>
> diff --git a/src/modules/module-mmkbd-evdev.c b/src/modules/module-mmkbd-evdev.c
> index 9ab7eb9..0b04f0f 100644
> --- a/src/modules/module-mmkbd-evdev.c
> +++ b/src/modules/module-mmkbd-evdev.c
> @@ -256,8 +256,11 @@ void pa__done(pa_module*m) {
>       if (u->io)
>           m->core->mainloop->io_free(u->io);
>   
> -    if (u->fd >= 0)
> -        pa_assert_se(pa_close(u->fd) == 0);
> +    if (u->fd >= 0) {
> +        int r = pa_close(u->fd);
> +        if (r < 0) /* https://bugs.freedesktop.org/show_bug.cgi?id=80867 */
> +            pa_log("Closing fd failed: %s", pa_cstrerror(errno));
> +    }
>   
>       pa_xfree(u->sink_name);
>       pa_xfree(u);

-- 
Alexander E. Patrakov