From ada687a4cc078beec4e6d2cf5386b0423797d820 Mon Sep 17 00:00:00 2001
From: Pauli Virtanen <pav@iki.fi>
Date: Sun, 15 Dec 2024 12:47:57 +0200
Subject: [PATCH] monitors/alsa: decouple name deduplication from node objects

Node name deduplication relying on presence of node objects creates race
conditions, as the name cannot be marked unused if the node object was
not created or was destroyed.

Use separate (device_id, node_id) -> name table to track name ownership
separately from the existence of node objects.

Also clear up the reserved names when device is destroyed, by the
monitor or device reservation.  In these cases "object-removed" for
nodes is not called, so this fixes names leaking when e.g. pw-reserve is
used.
---
 src/scripts/monitors/alsa.lua | 56 ++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/src/scripts/monitors/alsa.lua b/src/scripts/monitors/alsa.lua
index f6b20a89..6b0fc51b 100644
--- a/src/scripts/monitors/alsa.lua
+++ b/src/scripts/monitors/alsa.lua
@@ -17,6 +17,10 @@ config.rules = Conf.get_section_as_json ("monitor.alsa.rules", Json.Array {})
 device_names_table = nil
 node_names_table = nil
 
+-- SPA ids to node names: name = id_name_table[device_id][node_id]
+id_name_table = nil
+
+
 function nonempty(str)
   return str ~= "" and str or nil
 end
@@ -30,6 +34,7 @@ end
 
 function createNode(parent, id, obj_type, factory, properties)
   local dev_props = parent.properties
+  local parent_id = tonumber(dev_props["spa.object.id"])
 
   -- set the device id and spa factory name; REQUIRED, do not change
   properties["device.id"] = parent["bound-id"]
@@ -103,7 +108,6 @@ function createNode(parent, id, obj_type, factory, properties)
     -- deduplicate nodes with the same name
     for counter = 2, 99, 1 do
       if node_names_table[properties["node.name"]] ~= true then
-        node_names_table[properties["node.name"]] = true
         break
       end
       properties["node.name"] = name .. "." .. counter
@@ -161,43 +165,41 @@ function createNode(parent, id, obj_type, factory, properties)
 
   if cutils.parseBool (properties ["node.disabled"]) then
     log:notice ("ALSA node " .. properties["node.name"] .. " disabled")
-    node_names_table [properties ["node.name"]] = nil
     return
   end
 
+  node_names_table[properties["node.name"]] = true
+  id_name_table[parent_id][id] = properties["node.name"]
+
   -- create the node
   local node = Node("adapter", properties)
   node:activate(Feature.Proxy.BOUND, function (_, err)
       if err then
         log:warning ("Failed to create " .. properties ["node.name"]
           .. ": " .. tostring(err))
-
-        -- if it fails, object-removed gets called with missing
-        -- properties, so clean up already here
-        node_names_table [properties ["node.name"]] = nil
       end
   end)
   parent:store_managed_object(id, node)
 end
 
+function removeNode(parent, id)
+  local parent_id = tonumber(parent.properties["spa.object.id"])
+  local node_name = id_name_table[parent_id][id]
+
+  if node_name ~= nil then
+    log:info ("Removing node " .. node_name)
+    node_names_table[node_name] = nil
+    id_name_table[parent_id][id] = nil
+  end
+end
+
 function createDevice(parent, id, factory, properties)
+  id_name_table[id] = {}
+  properties["spa.object.id"] = id
   local device = SpaDevice(factory, properties)
   if device then
     device:connect("create-object", createNode)
-    device:connect("object-removed", function (parent, id)
-      local node = parent:get_managed_object(id)
-      if not node then
-        return
-      end
-
-      local node_name = node.properties["node.name"]
-      if node_name ~= nil then
-        log:info ("Removing node " .. node_name)
-        node_names_table[node_name] = nil
-      else
-        log:info ("Removing node with missing node.name")
-      end
-    end)
+    device:connect("object-removed", removeNode)
     device:activate(Feature.SpaDevice.ENABLED | Feature.Proxy.BOUND)
     parent:store_managed_object(id, device)
   else
@@ -205,6 +207,16 @@ function createDevice(parent, id, factory, properties)
   end
 end
 
+function removeDevice(parent, id)
+  if id_name_table[id] ~= nil then
+    for _, node_name in pairs(id_name_table[id]) do
+      log:info ("Release " .. node_name)
+      node_names_table[node_name] = nil
+    end
+    id_name_table[id] = nil
+  end
+end
+
 function prepareDevice(parent, id, obj_type, factory, properties)
   -- ensure the device has an appropriate name
   local name = "alsa_card." ..
@@ -317,6 +329,7 @@ function prepareDevice(parent, id, obj_type, factory, properties)
 
       elseif state == "busy" then
         -- destroy the device
+        removeDevice(parent, id)
         parent:store_managed_object(id, nil)
       end
     end)
@@ -347,6 +360,8 @@ function createMonitor ()
 
   -- handle object-removed to destroy device reservations and recycle device name
   m:connect("object-removed", function (parent, id)
+    removeDevice(parent, id)
+
     local device = parent:get_managed_object(id)
     if not device then
       return
@@ -367,6 +382,7 @@ function createMonitor ()
   -- reset the name tables to make sure names are recycled
   device_names_table = {}
   node_names_table = {}
+  id_name_table = {}
 
   -- activate monitor
   log:info("Activating ALSA monitor")