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

Async uevent #1298

Merged
merged 8 commits into from
Feb 21, 2025
Merged

Async uevent #1298

merged 8 commits into from
Feb 21, 2025

Conversation

michaelweiser
Copy link
Contributor

Thanks for this awesome library!

Here are some (hopefully self-explanatory) extensions to uevent handling. I use them to listen for backlight changes. Would you be interested in including those? What would potentially need doing to get them in?

@michaelweiser michaelweiser force-pushed the async-uevent branch 2 times, most recently from 757fc0c to 2411fee Compare February 20, 2025 19:20
@svinota
Copy link
Owner

svinota commented Feb 20, 2025

Great, thanks for the contribution!

Would be nice to update the PR:

  • the code should pass make format w/o errors — actually, make format tries to reformat the code, see git diff after running make format
  • [optional but something to think about] add some functional test, maybe using mock; see /noxfile.py and /tests
  • [optional but really nice] add types annotation and include the affected file into /.mypy-check-paths

Add an sync socket as thin layer on top of AsyncNetlinkSocket analogous
to UeventSocket on top of NetlinkSocket.
Uevent has no multipart messages with NLMSG_DONE at the end. So have
get() yield messages individually until all are consumed. Avoids error:

File "pyroute2/pyroute2/netlink/marshal.py", line 134, in is_enough
    return msg['header']['type'] == NLMSG_DONE
           ~~~~~~~~~~~~~^^^^^^^^
KeyError: 'type'
udevd sends messages to libudev consumers on the same socket as the
kernel but with a different group. Handle the different format with
binary header.

See: https://insujang.github.io/2018-11-27/udev-device-manager-for-the-linux-kernel-in-userspace/#libudev-and-kobject_uevent-message-groups
Although unlikely, it seems a good measure to account for possible
decoding errors in marshalling.
Define kernel and udevd message group constants and allow to specify
them on bind.
@svinota
Copy link
Owner

svinota commented Feb 20, 2025

There is an issue with the CI, so some jobs are failing; I'm to fix that tonight

@michaelweiser
Copy link
Contributor Author

make format is done but mypy causes some scope creep as it seems I need to put type annotations on all the superclasses as well to avoid "incompatible signature" errors. I'll play with it some more to see where it ends. :)

@svinota
Copy link
Owner

svinota commented Feb 20, 2025

The CI is fixed.

We can merge the code, and the annotations, and occasionally some tests you can push later as a separate PR.

Do not reassign variables to avoid type conflict reports from mypy. Find
a more efficient way for splitting lines into key and value while at it.

Turn parse() on MarshalUevent into a generator so it matches the
superclass' signature.  Not sure if this is still semantically correct,
the return in the error case and Generator SendType and ReturnType in
particular. (my generator-fu is weak)

Add superclass parameters to parse() to match signature (and ignore
them).

Annotating bind() on UeventSocket seems to work because mypy can't
follow the *argv, **kwargs on the superclass back into
AsyncNetlinkSocket.

No such luck on AsyncUeventSocket: While bind()'s signature can be made
compatible with that of AsyncNetlinkSocket, the one of AsyncCoreSocket
is plain incompatible (addr arg). So we basically have to promise mypy
that we know what we're doing at this point. See
https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
@michaelweiser
Copy link
Contributor Author

Here's an attempt at typing. Way more than what I bargained for. :)

Add two test messages to test_marshal, one from kernel and one from
udevd.
@svinota
Copy link
Owner

svinota commented Feb 21, 2025

Great! Thanks a lot!

@svinota svinota merged commit 4bc7cb5 into svinota:master Feb 21, 2025
21 checks passed
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.

2 participants