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

Remove duplicated alarm syscall. #636

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

zhalvorsen
Copy link
Collaborator

The alarm syscall is implemented in libtock-rs, but was duplicated here. This removes the duplicated code and changes the references to point to libtock-rs directly.

@zhalvorsen zhalvorsen requested a review from ia0 June 27, 2023 21:43
@coveralls
Copy link

coveralls commented Jun 27, 2023

Coverage Status

coverage: 96.333%. remained the same when pulling 2199f2f on zhalvorsen:remove_alarm into f0e87ee on google:develop.

The alarm syscall is implemented in libtock-rs, but was duplicated here.
This removes the duplicated code and changes the references to point to
libtock-rs directly.
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

I tested on webauthn (register, login, reset, login) and it works.

However, I was able to generate a panic too (but unrelated to this PR):

panicked at 'assertion failed: self.pending_out.take()', capsules/src/usb/usbc_ctap_hid.rs:364:17

I don't really have reproduction steps because it looks like a timing issue. Here are the steps I'm taking:

  • Have a nordic dev board with opensk and another security key (U2F only) connected (not sure it matters, but that's the setup)
  • Register both.
  • Authenticate repeatedly (cancelling the popup and not touching any key)
  • The dev board will eventually panic when clicking Authenticate (not when cancelling the popup)

Copy link
Collaborator

@kaczmarczyck kaczmarczyck left a comment

Choose a reason for hiding this comment

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

If @ia0 's panic is unrelated to the PR, feel free to merge. We can always test and fix it more later.

@kaczmarczyck kaczmarczyck merged commit 8868752 into google:develop Jul 11, 2023
24 checks passed
@zhalvorsen zhalvorsen deleted the remove_alarm branch August 8, 2023 16:16
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.

4 participants