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

keyd: Verify current udevd process is supervised by runit #38467

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

Barbaross93
Copy link
Contributor

@Barbaross93 Barbaross93 commented Aug 4, 2022

Testing the changes

  • I tested the changes in this PR: YES

This PR adds a check to the runit service for keyd. Sometimes, when starting the keyd service, the keyboard can become unresponsive. This was the result of the udevd process being replaced while keyd was launching. See here for more details.

@akhiljalagam
Copy link
Contributor

keyboard can become unresponsive.
true.
I disabled keyd runit service and starting manully as a workaround.

@akhiljalagam
Copy link
Contributor

rvaiya/keyd#263 (comment)
I don't understand why udevd starting 2 times.
Is it necessary?

@Barbaross93
Copy link
Contributor Author

I'm not sure either @akhiljalagam . I'm hoping a void maintainer can chime in here.

@Duncaen
Copy link
Member

Duncaen commented Sep 12, 2022

I don't understand why udevd starting 2 times.

Because udevd is needed early for early initialization at a point where runit services are not running. Its replaced by the runit service so its supervised by runit and not just some random daemon in the background.

Could you elaborate on what doesn't make sense?

The whole description of the problem and what the check does.

@Barbaross93
Copy link
Contributor Author

The problem boils down to a race condition. Sometimes, but not always, the keyd program is launched but the keyboard is unresponsive until the process is terminated. Based on the linked issue, the reason for this is because keyd process launched after the early udevd process, but before the udevd runit service. So, when the runit service starts for udevd, it kills the early udevd process causing keyd to stop working as expected.

This PR adds a check to ensure that keyd doesn't launch until the the udevd runit service has launched.

@Duncaen
Copy link
Member

Duncaen commented Sep 12, 2022

This should be mentioned in the services, otherwise the "check" makes no sense at all.

@Barbaross93
Copy link
Contributor Author

This should be mentioned in the services

Are you suggesting that I should merely leave a comment in the service file about this issue? Won't peoples custom solutions be overwritten on updates then?

The check verifies that the supervised udevd process is the current running udevd process. I thought it was pretty self-explanatory.

@Duncaen
Copy link
Member

Duncaen commented Sep 12, 2022

Are you suggesting that I should merely leave a comment in the service file about this issue? Won't peoples custom solutions be overwritten on updates then?

No there should be a comment to why this check exist.

The check verifies that the supervised udevd process is the current running udevd process. I thought it was pretty self-explanatory.

nobody will know why this check exists in the first place without having to track down the PR where it has been introduced.
The commit just says "add check to runit service" and the line alone makes not much sense without knowing the full context and specifics about how udevd is spawned twice without a comment that mentions that.

@Barbaross93
Copy link
Contributor Author

No there should be a comment to why this check exist.

OK, thank you, this makes much more sense.

nobody will know why this check exists in the first place without having to track down the PR where it has been introduced.
The commit just says "add check to runit service" and the line alone makes not much sense without knowing the full context and specifics about how udevd is spawned twice without a comment that mentions that.

This also makes much more sense. Will it be sufficient to edit the commit message in addition to a comment?

@Duncaen
Copy link
Member

Duncaen commented Sep 12, 2022

This also makes much more sense. Will it be sufficient to edit the commit message in addition to a comment?

Should probably be in both, because its not clear from just the command compared to sv check.

@Barbaross93 Barbaross93 changed the title keyd: add check to runit service keyd: Verify current udevd process is supervised by runit Sep 12, 2022
@Barbaross93
Copy link
Contributor Author

Barbaross93 commented Sep 12, 2022

Sounds good.

The github checks: I'm not sure why they're failing?
Looks like there was a temporary blip there.

Sometimes when starting the keyd service, the keyboard can become unresponsive.
This is the result of keyd starting when the early udevd process is still
running but before the udevd runit service has started. The udevd runit
service kills the early udevd process causing the keyboard to become
unresponsive until the keyd process has been terminated. The below
code verifies that the supervised udevd process is the same as the
currently running udevd process. Please enter the commit message for
your changes. Lines starting with '#' will be ignored, and an empty
message aborts the commit.
@Barbaross93
Copy link
Contributor Author

@Duncaen I think this is all set? Let me know if I missed something.

@Duncaen Duncaen merged commit dbc1fb5 into void-linux:master Oct 5, 2022
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.

3 participants