[PATCH v4 0/5] MR5385: winebus: ignore udev events of devices which do not have devnodes

[Tuomas Räsänen (＠tuomasjjrasanen)]

Udev monitor monitors the whole input subsystem, but not all devices
in the input subsystem have devnodes associated to them.

This MR makes the event processing ignore such devices.

All device handling assumes devices have devnodes, so here we just
simply ignore all devices which do not have one. They are irrelevant.

Previously, udev bus thread aborted when an event for a device without
a devnode was processed:

```
  10111.330:0068:0084:trace:hid:process_monitor_event Received action "remove" for udev device (null)
  10111.330:0068:0084:warn:hid:bus_main_thread L"UDEV" bus wait returned status 0xc0000005
```

Just plugging in and out a normal mouse was enough cause this.

This was because root input devices (which do not have devnodes) were
handled too and `find_device_from_devnode()` choked on NULL argument.

--
  v4: winebus: change debugging class of an error case from WARN to ERR.
      winebus: change debugging class of an error case from FIXME to ERR
      winebus: group local variable declarations

https://gitlab.winehq.org/wine/wine/-/merge_requests/5385

[PATCH v4 1/5] winebus: log ERR if find_device_from_devnode() somehow gets called with NULL

[Tuomas Räsänen]

From: Tuomas Räsänen

---
 dlls/winebus.sys/bus_udev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index 419de2e7af3..da733838dcb 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -264,6 +264,12 @@ static struct base_device *find_device_from_devnode(const char *path)
 {
     struct base_device *impl;

+    if (!path)
+    {
+        ERR("called find_device_from_devnode(NULL)\n");
+        return NULL;
+    }
+
     LIST_FOR_EACH_ENTRY(impl, &device_list, struct base_device, unix_device.entry)
         if (!strcmp(impl->devnode, path)) return impl;

[PATCH v4 2/5] winebus: ignore udev events of devices which do not have devnodes

[Tuomas Räsänen]

From: Tuomas Räsänen

Udev monitor monitors the whole input subsystem, but not all devices
in the input subsystem have devnodes associated to them.

This commit makes the event processing ignore such devices.

All device handling assumes devices have devnodes, so here we just
simply ignore all devices which do not have one.

Previously, udev bus thread aborted when an event for a device without
a devnode was processed:

10111.330:0068:0084:trace:hid:process_monitor_event Received action "remove" for udev device (null)
  10111.330:0068:0084:warn:hid:bus_main_thread L"UDEV" bus wait returned status 0xc0000005

Just plugging in and out a normal mouse was enough cause this.

This was because root input devices (which do not have devnodes) were
handled too and find_device_from_devnode() choked on NULL argument.
---
 dlls/winebus.sys/bus_udev.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index da733838dcb..e90a3abfdc3 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -1717,6 +1717,8 @@ static void process_monitor_event(struct udev_monitor *monitor)
     struct base_device *impl;
     struct udev_device *dev;
     const char *action;
+    const char *devnode;
+    const char *syspath;

dev = udev_monitor_receive_device(monitor);
     if (!dev)
@@ -1726,16 +1728,28 @@ static void process_monitor_event(struct udev_monitor *monitor)
     }

action = udev_device_get_action(dev);
-    TRACE("Received action %s for udev device %s\n", debugstr_a(action),
-          debugstr_a(udev_device_get_devnode(dev)));
-
-    if (!action)
-        WARN("No action received\n");
+    syspath = udev_device_get_syspath(dev);
+    devnode = udev_device_get_devnode(dev);
+    TRACE("Received action %s for udev device %s (%p) devnode %s\n",
+          debugstr_a(action), debugstr_a(syspath), dev, debugstr_a(devnode));
+
+    if (!syspath)
+        ERR("udev device %p does not have syspath!\n", dev);
+    else if (!action)
+        WARN("event for udev device %s does not have any action!\n", syspath);
+    else if (!devnode)
+    {
+        /* Pretty normal case, not all devices have associated
+         * devnodes. For example root input devices do not, but
+         * related/child mouse and event devices do.
+         */
+        TRACE("udev device %s does not have devnode, ignoring\n", syspath);
+    }
     else if (strcmp(action, "remove"))
         udev_add_device(dev, -1);
     else
     {
-        impl = find_device_from_devnode(udev_device_get_devnode(dev));
+        impl = find_device_from_devnode(devnode);
         if (impl) bus_event_queue_device_removed(&event_queue, &impl->unix_device);
         else WARN("failed to find device for udev device %p\n", dev);
     }

[PATCH v4 3/5] winebus: group local variable declarations

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/winebus.sys/bus_udev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index e90a3abfdc3..92ad4b3bb79 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -1716,9 +1716,7 @@ static void process_monitor_event(struct udev_monitor *monitor)
 {
     struct base_device *impl;
     struct udev_device *dev;
-    const char *action;
-    const char *devnode;
-    const char *syspath;
+    const char *action, *devnode, *syspath;

dev = udev_monitor_receive_device(monitor);
     if (!dev)

[PATCH v4 4/5] winebus: change debugging class of an error case from FIXME to ERR

[Tuomas Räsänen]

From: Tuomas Räsänen

---
 dlls/winebus.sys/bus_udev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index 92ad4b3bb79..7013093db79 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -1721,7 +1721,7 @@ static void process_monitor_event(struct udev_monitor *monitor)
     dev = udev_monitor_receive_device(monitor);
     if (!dev)
     {
-        FIXME("Failed to get device that has changed\n");
+        ERR("Failed to get device that has changed\n");
         return;
     }

[PATCH v4 5/5] winebus: change debugging class of an error case from WARN to ERR.

[Tuomas Räsänen]

From: Tuomas Räsänen

---
 dlls/winebus.sys/bus_udev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index 7013093db79..02dfd35c789 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -1734,7 +1734,7 @@ static void process_monitor_event(struct udev_monitor *monitor)
     if (!syspath)
         ERR("udev device %p does not have syspath!\n", dev);
     else if (!action)
-        WARN("event for udev device %s does not have any action!\n", syspath);
+        ERR("event for udev device %s does not have any action!\n", syspath);
     else if (!devnode)
     {
         /* Pretty normal case, not all devices have associated

[Rémi Bernon (＠rbernon)]

On Mon Mar 25 18:53:47 2024 +0000, Tuomas Räsänen wrote:
> Just fixed the commit the use ERR instead, but it's your call whether
> this should be included or not. I think yes with the reasoning above.

I don't think it is useful, it's already checked elsewhere and the helper is simple enough to show that it does not accept NULL, and that's up to the caller.

[Rémi Bernon (＠rbernon)]

Looks okay otherwise but please squash the changes together.

[PATCH v4 0/1] MR5385: winebus: ignore udev events of devices which do not have devnodes

[Tuomas Räsänen (＠tuomasjjrasanen)]

On Mon Mar 25 20:30:14 2024 +0000, Rémi Bernon wrote:
> I don't think it is useful, it's already checked elsewhere and the
> helper is simple enough to show that it does not accept NULL, and that's
> up to the caller.

I'll drop the first commit then.