[PATCH v3 0/5] MR5385: winebus: ignore udev events of devices which do not have devnodes
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.
-- v3: 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 winebus: ignore udev events of devices which do not have devnodes winebus: log ERR if find_device_from_devnode() somehow gets called with NULL
From: Tuomas Räsänen tuomas.rasanen@opinsys.fi
--- 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;
From: Tuomas Räsänen tuomas.rasanen@opinsys.fi
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); }
From: Rémi Bernon rbernon@codeweavers.com
--- 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..11ce75d811f 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)
From: Tuomas Räsänen tuomas.rasanen@opinsys.fi
--- 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 11ce75d811f..0843f5ddcd9 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; }
From: Tuomas Räsänen tuomas.rasanen@opinsys.fi
--- 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 0843f5ddcd9..358a6f8bb1f 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
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=144383
Your paranoid android.
=== debian11 (build log) ===
../wine/dlls/winebus.sys/bus_udev.c:1729:13: error: assignment of read-only variable ���syspath��� ../wine/dlls/winebus.sys/bus_udev.c:1730:13: error: assignment of read-only variable ���devnode��� Task: The win32 Wine build failed
=== debian11b (build log) ===
../wine/dlls/winebus.sys/bus_udev.c:1729:13: error: assignment of read-only variable ���syspath��� ../wine/dlls/winebus.sys/bus_udev.c:1730:13: error: assignment of read-only variable ���devnode��� Task: The wow64 Wine build failed
On Mon Mar 25 17:27:07 2024 +0000, Tuomas Räsänen wrote:
I thought that it would be good idea to add this log statement regardless of the current usage of `find_device_from_devnode()`, to make it "future-proof": whenever this function gets called with NULL argument, it's always an error. Had it been there, I would have found the issue a bit faster. Now the log contained only `warn:hid:bus_main_thread L"UDEV" bus wait returned status 0xc0000005`. The code `0xc0000005` of course hinted that an invalid pointer is being used, but it was not immediately clear where. And, yes, ERR would be then better than FIXME. I think I just mimiced some existing error condition, the FIXME usage does not seem to be entirely consistent.
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.
On Mon Mar 25 18:40:50 2024 +0000, Tuomas Räsänen wrote:
I think the `!syspath` case deserves `ERR`: all devices in sysfs tree always have unique paths. If udev abstraction somehow fails to convey it, then something is really broken and basically all bets are off regarding current device state. I think `!action` case is similar; it means that something has happened in the device setup but udev has failed to tell us what. That case was logged as WARN and I didn't want to touch that because it was not necessary in the context of this MR. Anyways, point taken, I'll get rid of "on error goto out" -pattern (which I personally tend to prefer).
I just fixed the commit to not use "on error goto out" -pattern but use ERR for `!syspath` case.
I also added a commit which changes WARN to ERR on in `!action` case.
Tuomas Räsänen (@tuomasjjrasanen) commented about dlls/winebus.sys/bus_udev.c:
{ struct base_device *impl; struct udev_device *dev;
- const char *action;
const char *action, devnode, syspath;
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;
I also added a commit which changes this existing FIXME to ERR. I think this was the case which lead me to use FIXME incorrectly in the first commit.
-
Marvin -
Rémi Bernon -
Tuomas Räsänen
-
Tuomas Räsänen (＠tuomasjjrasanen)