Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New method for finding the device node corresponding to a uinput device #206

Merged
merged 9 commits into from
Jan 9, 2024

Conversation

KarsMulder
Copy link
Contributor

As discussed in issue #205. Finds the device node on Linux systems that support UI_GET_SYSNAME in the same way that libevdev does.

I haven't implemented the FreeBSD variant of the logic yet because (1) then I'd have to set up a FreeBSD VM to test it, and (2) the documentation of python-evdev does not even mention FreeBSD as a supported platform. That said, the FreeBSD variant of the function _find_device_linux should literally be a simple as:

def _find_device_freebsd(self, sysname):
    return device.InputDevice(f'/dev/input/{sysname}')

There is some discussion about how the code of _find_device_fallback should be updated in issue #204 and #205. This pull request does not integrate those proposed changes.

The old method of scanning the filesystem to find an event device with a
matching name was prone to race conditions, required a time.sleep delay,
unnecessarily opened many event devices, and had other fixable issues.

The new UI_GET_SYSNAME-based method is immune to race conditions and
does not require time.sleep(), while also not suffering from the
aforementioned fixable issues.
@KarsMulder
Copy link
Contributor Author

I just realized that the claim that the new method "does not need the time.sleep" in the commit message is wrong. Or at least, there is no fundamental issue that has been fixed by the new method: even if the new method can find the path to the event device before it shows up in /dev/input, it will not actually be able to open the device if it hasn't shown up yet.

On my current system, no delay is needed. The commit that added the delay was c289d68, which was made in 2012. I suppose that in the traditional way of letting /dev be managed by udev, there is need for a delay, but on modern systems where /dev and /sys are managed by the kernel through devtmpfs and sysfs, the device nodes should show up instantaneously.

We could maintain backwards compatibility with operating systems that still populate /dev through udev by simply moving the time.sleep(0.1) to the top of _find_device. The less simple but more performant way would be to check if we can open the device node without delay, and then wait 0.1s and retry if that fails. But in that case, do we need the try-otherwise-delay guard on only the access to /dev, or do we need it on the access to /sys as well? Are there any Linux systems on which /sys is not instantaneously populated by the kernel?

(It is now too late for me to dig into this further right now.)

Copy link
Collaborator

@sezanzeb sezanzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution

evdev/uinput.c Show resolved Hide resolved
evdev/uinput.py Show resolved Hide resolved
evdev/uinput.py Outdated Show resolved Hide resolved
evdev/uinput.c Outdated Show resolved Hide resolved
@sezanzeb
Copy link
Collaborator

sezanzeb commented Jan 6, 2024

We could maintain backwards compatibility with operating systems that still populate /dev through udev by simply moving the time.sleep(0.1) to the top of _find_device. The less simple but more performant way would be to check if we can open the device node without delay, and then wait 0.1s and retry if that fails. But in that case, do we need the try-otherwise-delay guard on only the access to /dev, or do we need it on the access to /sys as well? Are there any Linux systems on which /sys is not instantaneously populated by the kernel?

No clue.

Retry logic isn't nice to implement, and also seems to be somewhat of a dirty solution to me, as opposed to carefully checking if stuff exists before doing anything:

    def _wait_if_doesnt_exist(self, path):
        if !os.path.exists(path):
            #:bug: the device node might not be immediately available
            # On modern systems where /dev and /sys are managed by the kernel through
            # devtmpfs and sysfs, the device nodes should show up instantaneously.
            time.sleep(0.1)

    def _find_device_linux(self, sysname):
        '''
        Tries to find the device node when running on Linux.
        '''

        syspath = f'/sys/devices/virtual/input/{sysname}'
        self._wait_if_doesnt_exist(syspath)

        # The sysfs entry for event devices should contain exactly one folder
        # whose name matches the format "event[0-9]+". It is then assumed that
        # the device node in /dev/input uses the same name.
        regex = re.compile('event[0-9]+')
        for entry in os.listdir(syspath):
            if regex.fullmatch(entry):
                device_path = f'/dev/input/{entry}'
                self._wait_if_doesnt_exist(device_path)
                return device.InputDevice(device_path)

        raise FileNotFoundError()

I guess we could do that, better safe than sorry

@KarsMulder
Copy link
Contributor Author

KarsMulder commented Jan 6, 2024

Retry logic isn't nice to implement, and also seems to be somewhat of a dirty solution to me,

It's actually the other way around: catching FileNotFoundError is the proper way of working with filesystems, and using os.path.exists() is the dirty solution. Catching exceptions is usually better than using os.path.exists because all filesystem accesses are inherently subject to race conditions from other processes. Consider the following code:

def maybe_read_file(path):
    if not os.path.exists(path):
        handle_nonexistent_file(path)
    with open(path, 'r') as file:
        do_something(file)

The problem is that somewhere in the short time between the os.path.exists() call and the open() call, another process might have deleted the file in the meanwhile, leading to an error cause where you neither opened the file nor called the handle_nonexistent_file() handle that was meant to be called. This is called a time-of-check time-of-use bug.

As such, checking whether a file exists and then opening it is considered to be a bad practice; it is almost always preferable to just try to open the file and then handle a FileNotFoundError.

(In this specific case, if the file mysteriously disappears between the os.path.exists() call and the open() call, we've got bigger problems to deal with that a time.sleep() won't solve, but I do want to clarify that there is nothing dirty about catching FileNotFoundError instead of checking if a file exists.)

Unlike what a previous commit claimed, the new method is only guaranteed
to find the device name instantaneously, but if /dev is managed by udev,
the corresponding /dev/input/input* file might not exist immediately. On
modern Linux systems, those files are managed by devtmpfs so they do
show up immediately and it would be a waste of time to sleep, but we do
want a fallback time.sleep() call for those operating systems which do
not use devtmpfs.

It is assumed that on any linux system, /sys is managed by sysfs; it
conceptually doesn't really make sense to let that directory be managed
by any other filesystem because the files in /sys are basically
syscalls. As such, we assume that /sys always updates instantaneously,
and if for whatever reason it doesn't, _find_device_fallback shall be
used instead.
@sezanzeb
Copy link
Collaborator

sezanzeb commented Jan 7, 2024

As such, checking whether a file exists and then opening it is considered to be a bad practice; it is almost always preferable to just try to open the file and then handle a FileNotFoundError.

Thanks for explaining

evdev/uinput.py Show resolved Hide resolved
evdev/uinput.py Show resolved Hide resolved
@sezanzeb
Copy link
Collaborator

sezanzeb commented Jan 7, 2024

This decorator could be used universally for both _find_device functions. Since it wraps everything, it would conveniently cover both /sys and /dev

def retry(find_device):
    def wrapped(*args, **kwargs):
        try:
            return find_device(*args, **kwargs)
        except FileNotFoundError:
            # It is possible that there is some delay before /dev/input/event* shows
            # up on old systems that do not use devtmpfs, so if the device cannot be
            # found, wait for a short amount and then try again once.
            time.sleep(0.1)
            return find_device(*args, **kwargs)
    return wrapped

class UInput(EventIO):
    ...

    @retry
    def _find_device_fallback(self):
        ...

    @retry
    def _find_device_linux(self, sysname):
        ...

The best solution would probably be to figure out which kernel version fixes this issue, and then compare that with os.uname().release, which needs to be parsed. Or maybe that kernel has reached end of life years ago anyway.

@sezanzeb
Copy link
Collaborator

sezanzeb commented Jan 7, 2024

devtmpfs was added Thu, 30 Apr 2009 or something: https://lwn.net/Articles/330985/


https://unix.stackexchange.com/a/77936:

And I see that it is enabled by default in the Debian distribution kernel 3.2.0-4-amd64

This kernel has reached EOL mid 2020: https://en.wikipedia.org/wiki/Linux_kernel_version_history

And kernel 2.*, where debian definitely hasn't had this by default, has reached EOL early 2016


https://unix.stackexchange.com/a/236901:

Sysfs is automatically populated by the kernel, reflecting the actually available devices in real time.

devtmpfs, which is an instance of tmpfs where entries for available devices are automatically created by the kernel, but udev does all the management it wants on top of that.


For my naive understanding this means that the creation of an UInput leads to the creation of the devnodes immediately. udev is just additional stuff on top of it nowadays. And all distros have probably been shipping devtmpfs and sysfs for years. It certainly seems to be on on my system:

➜  ~ cat /boot/config-$(uname -r) | ack CONFIG_DEVTMPFS
CONFIG_DEVTMPFS=y
CONFIG_DEVTMPFS_MOUNT=y
CONFIG_DEVTMPFS_SAFE=y

@sezanzeb
Copy link
Collaborator

sezanzeb commented Jan 7, 2024

Checking if CONFIG_DEVTMPFS is set might be good, but I don't feel so well about looking into config files for this purpose, it should be available via some built-in python utility. And since this configuration variable could be found on my system, I'm assuming it is still possible to disable it.

So maybe something like

if int(os.uname().release[0]) < 4:
    # It is possible that there is some delay before /dev/input/event* shows
    # up on old systems that do not use devtmpfs, so if the device cannot be
    # found, wait for a short amount and then try again once.
    time.sleep(0.1)

wouldn't be 100% safe.

mount | grep /dev can tell you if devtmpfs is being used. But I'd rather have some built-in python function for this instead of creating a new subprocess. Unfortunately, those two options

>>> os.stat('/dev')
>>> os.statvfs('/dev')

don't report st_fstype, and it almost sounds like this is specific to solaris python/cpython#86339

idk. If you like the solution using a decorator, maybe lets just do that.

Co-authored-by: Tobi <28510156+sezanzeb@users.noreply.github.com>
@KarsMulder
Copy link
Contributor Author

KarsMulder commented Jan 7, 2024

This decorator could be used universally for both _find_device functions. Since it wraps everything, it would conveniently cover both /sys and /dev

There is an issue with this decorator: while it would achieve the intended result for _find_device_linux, it cannot be used for _find_device_fallback, because the fallback code will return the newest device with the correct name. If the device it is supposed to find hasn't shown up yet but there is another device available with the same name, then it will return that other device instead of raising FileNotFoundError.

(Actually, _find_device_fallback has issues either way—that's why _find_device_linux was written. If the operating system does not use devtmpfs, then we would suffer from a race condition in _find_device_fallback if it does not unconditionally sleep. However, if it does unconditionally sleep, then we will increasingly likely suffer from a race condition in case some init system starts two python-evdev scripts simultaneously.)

@sezanzeb
Copy link
Collaborator

sezanzeb commented Jan 7, 2024

because the fallback code will return the newest device with the correct name. If the device it is supposed to find hasn't shown up yet but there is another device available with the same name, then it will return that other device instead of raising FileNotFoundError.

So we always delay to be sure this doesn't happen?

I'll run some tests with the changes soon, thanks.

Since Python 3.3 (PEP 3151), IOError and OSError are aliases of each
other, and OSError is the prefered name. Rename all mentions of IOError
in the C bindings with OSError.
@KarsMulder
Copy link
Contributor Author

So we always delay to be sure this doesn't happen?

If we want issue #205 to be resolved on archaic operating systems without devtmpfs, then yes.

Copy link
Collaborator

@sezanzeb sezanzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work. I'd say this is ready to merge.

I thought we might just want to add another comment, why the sorting is necessary.

If there is no reason for delay, I'll merge it afterwards.

evdev/uinput.py Show resolved Hide resolved
Co-authored-by: Tobi <28510156+sezanzeb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants