From 360e1fbada721a21a725b003a928263f49aa5b93 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= <daniel@ekloef.se>
Date: Sat, 21 Aug 2021 11:08:45 +0200
Subject: [PATCH] =?UTF-8?q?module/alsa:=20don=E2=80=99t=20re-create=20the?=
 =?UTF-8?q?=20/dev/snd=20inotify=20watcher=20after=20each=20connect=20fail?=
 =?UTF-8?q?ure?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When e.g. a USB soundcard is inserted, we get several CREATE
events. In my experiments, we only succeed in connecting to ALSA after
the last event.

This means, we’ll have several CREATE events that we receive, remove
the watcher, attempt to connect, fail, and then re-add the watcher.

What if that “last” CREATE event occurs while our watcher has been
removed? That’s right, we miss it, and will get stuck waiting forever.

The solution is keep the watcher around.

Now, if we’ve been successfully connected to ALSA for a long time,
chances are we’ve built up events (for other cards, for example). We
don’t want to trigger a storm of re-connect attempts, so drain the
event queue after having been disconnected from ALSA.

There *is* a small race here - if a card is removed and
re-added *very* fast, we _may_ accidentally drain the CREATE event. I
don’t see this happening in reality though.
---
 modules/alsa.c | 79 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 25 deletions(-)

diff --git a/modules/alsa.c b/modules/alsa.c
index aa985b0..368b1ee 100644
--- a/modules/alsa.c
+++ b/modules/alsa.c
@@ -318,38 +318,64 @@ run(struct module *mod)
 {
     int ret = 1;
 
-    int wd = -1;
-    int ifd = inotify_init();
+    int ifd = inotify_init1(IN_NONBLOCK | IN_CLOEXEC);
     if (ifd < 0) {
         LOG_ERRNO("failed to inotify");
         return 1;
     }
 
+    int wd = inotify_add_watch(ifd, "/dev/snd", IN_CREATE);
+    if (wd < 0) {
+        LOG_ERRNO("failed to create inotify watcher for /dev/snd");
+        close(ifd);
+        return 1;
+    }
+
     while (true) {
         enum run_state state = run_while_online(mod);
 
         switch (state) {
         case RUN_DONE:
             ret = 0;
-            break;
+            goto out;
 
         case RUN_ERROR:
             ret = 1;
-            break;
+            goto out;
 
         case RUN_FAILED_CONNECT:
+            break;
+
         case RUN_DISCONNECTED:
+            /*
+             * We’ve been connected - drain the watcher
+             *
+             * We don’t want old, un-releated events (for other
+             * soundcards, for example) to trigger a storm of
+             * re-connect attempts.
+             */
+            while (true) {
+                uint8_t buf[1024];
+                ssize_t amount = read(ifd, buf, sizeof(buf));
+                if (amount < 0) {
+                    if (errno == EAGAIN)
+                        break;
+
+                    LOG_ERRNO("failed to drain inotify watcher");
+                    ret = 1;
+                    goto out;
+                }
+
+                if (amount == 0)
+                    break;
+            }
+
             break;
         }
 
-        wd = inotify_add_watch(ifd, "/dev/snd", IN_CREATE);
-        if (wd < 0) {
-            LOG_ERRNO("failed to create inotify watcher for /dev/snd");
-            ret = 1;
-            break;
-        }
+        bool have_create_event = false;
 
-        while (true) {
+        while (!have_create_event) {
             struct pollfd fds[] = {{.fd = mod->abort_fd, .events = POLLIN},
                                    {.fd = ifd, .events = POLLIN}};
             int r = poll(fds, sizeof(fds) / sizeof(fds[0]), -1);
@@ -363,42 +389,45 @@ run(struct module *mod)
                 goto out;
             }
 
-            if (fds[0].revents & POLLIN) {
+            if (fds[0].revents & (POLLIN | POLLHUP)) {
                 ret = 0;
                 goto out;
             }
 
-            if (fds[1].revents & POLLIN) {
+            if (fds[1].revents & POLLHUP) {
+                LOG_ERR("inotify socket closed");
+                ret = 1;
+                goto out;
+            }
+
+            assert(fds[1].revents & POLLIN);
+
+            while (true) {
                 char buf[1024];
                 ssize_t len = read(ifd, buf, sizeof(buf));
 
                 if (len < 0) {
+                    if (errno == EAGAIN)
+                        break;
+
                     LOG_ERRNO("failed to read inotify events");
                     ret = 1;
                     goto out;
                 }
 
-                if (len == 0) {
-                    LOG_ERR("inotify FD closed");
-                    ret = 1;
-                    goto out;
-                }
+                if (len == 0)
+                    break;
 
                 /* Consume inotify data */
-                bool have_create_event = false;
                 for (const char *ptr = buf; ptr < buf + len; ) {
                     const struct inotify_event *e = (const struct inotify_event *)ptr;
+
                     if (e->mask & IN_CREATE) {
                         LOG_DBG("inotify: CREATED: /dev/snd/%.*s", e->len, e->name);
                         have_create_event = true;
                     }
-                    ptr += sizeof(*e) + e->len;
-                }
 
-                if (have_create_event) {
-                    inotify_rm_watch(ifd, wd);
-                    wd = -1;
-                    break;
+                    ptr += sizeof(*e) + e->len;
                 }
             }
         }