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
