Skip to content

Simplify syscall condition and timeouts#1583

Merged
sporksmith merged 9 commits intoshadow:mainfrom
sporksmith:redundant-timer
Aug 20, 2021
Merged

Simplify syscall condition and timeouts#1583
sporksmith merged 9 commits intoshadow:mainfrom
sporksmith:redundant-timer

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

This is primarily to make timeout handling easier to expose in Rust.

  • Remove SyscallHandler::timer, since in practice it was redundant with SyscallCondition::timeout.
  • Deduplicate code to read and validate a timespec

Once a signal has been scheduled for a condition, never schedule another
signal for that condition, even after the first signal is delivered.

We never intentionally reuse a condition in practice, but the previous
behavior opened the door to confusing bugs if the condition *was*
triggered again in between its handler running and it being canceled.
This makes the triggering syscall condition, when applicable, available
to the handler.
We already have a timer in the SysCallCondition, which in practice was
always aliased to SysCallHandler.timer. Deduplicate, and encapsulate in
the SysCallCondition.
@sporksmith sporksmith self-assigned this Aug 19, 2021
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Aug 19, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 19, 2021

Codecov Report

Merging #1583 (0ad9a78) into main (b507cc1) will decrease coverage by 0.18%.
The diff coverage is 70.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1583      +/-   ##
==========================================
- Coverage   52.67%   52.48%   -0.19%     
==========================================
  Files         141      141              
  Lines       21262    21286      +24     
  Branches     5378     5380       +2     
==========================================
- Hits        11200    11173      -27     
- Misses       7089     7149      +60     
+ Partials     2973     2964       -9     
Flag Coverage Δ
tests 52.48% <70.29%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/syscall/uio.c 20.00% <0.00%> (ø)
src/main/host/syscall/unistd.c 49.26% <0.00%> (ø)
src/main/host/process.c 68.88% <27.27%> (-0.87%) ⬇️
src/main/host/syscall/protected.c 58.82% <58.82%> (-13.91%) ⬇️
src/main/host/syscall/futex.c 67.70% <75.00%> (+3.06%) ⬆️
src/main/host/syscall/socket.c 78.30% <75.00%> (ø)
src/main/host/syscall/time.c 63.04% <77.77%> (+3.46%) ⬆️
src/main/host/syscall_condition.c 69.71% <78.94%> (-0.85%) ⬇️
src/main/host/syscall/poll.c 81.57% <85.71%> (+0.32%) ⬆️
src/main/host/descriptor/timer.c 71.51% <100.00%> (-0.61%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b507cc1...0ad9a78. Read the comment docs.

@sporksmith sporksmith requested a review from robgjansen August 19, 2021 22:39
Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

Looks great!

@sporksmith sporksmith enabled auto-merge August 20, 2021 17:27
@sporksmith sporksmith merged commit d64d07c into shadow:main Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants