Skip to content

Comments

Save fifos#5698

Merged
JackThomson2 merged 3 commits intofirecracker-microvm:mainfrom
ShadowCurse:save_fifos
Feb 20, 2026
Merged

Save fifos#5698
JackThomson2 merged 3 commits intofirecracker-microvm:mainfrom
ShadowCurse:save_fifos

Conversation

@ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Feb 18, 2026

Changes

Partial revert of b656ae3 commit by readding the read flag to open files, since otherwise FIFOs will fail to open if there are no readers already attached to them.

Reason

Fix regression

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.
  • This functionality cannot be added in rust-vmm.

@ShadowCurse ShadowCurse self-assigned this Feb 18, 2026
@ShadowCurse ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Fix Indicates a fix to existing code labels Feb 18, 2026
@codecov
Copy link

codecov bot commented Feb 18, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.15%. Comparing base (992b70a) to head (8d28ab2).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/vmm/src/device_manager/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5698   +/-   ##
=======================================
  Coverage   83.15%   83.15%           
=======================================
  Files         277      277           
  Lines       29384    29385    +1     
=======================================
+ Hits        24435    24436    +1     
  Misses       4949     4949
Flag Coverage Δ
5.10-m5n.metal 83.45% <83.33%> (+<0.01%) ⬆️
5.10-m6a.metal 82.78% <83.33%> (+<0.01%) ⬆️
5.10-m6g.metal 80.15% <83.33%> (+<0.01%) ⬆️
5.10-m6i.metal 83.45% <83.33%> (+<0.01%) ⬆️
5.10-m7a.metal-48xl 82.78% <83.33%> (+<0.01%) ⬆️
5.10-m7g.metal 80.15% <83.33%> (+<0.01%) ⬆️
5.10-m7i.metal-24xl 83.43% <83.33%> (+<0.01%) ⬆️
5.10-m7i.metal-48xl 83.42% <83.33%> (+<0.01%) ⬆️
5.10-m8g.metal-24xl 80.15% <83.33%> (+<0.01%) ⬆️
5.10-m8g.metal-48xl 80.15% <83.33%> (+<0.01%) ⬆️
5.10-m8i.metal-48xl 83.43% <83.33%> (+<0.01%) ⬆️
5.10-m8i.metal-96xl 83.42% <83.33%> (-0.01%) ⬇️
6.1-m5n.metal 83.48% <83.33%> (+<0.01%) ⬆️
6.1-m6a.metal 82.81% <83.33%> (-0.01%) ⬇️
6.1-m6g.metal 80.15% <83.33%> (-0.01%) ⬇️
6.1-m6i.metal 83.48% <83.33%> (+<0.01%) ⬆️
6.1-m7a.metal-48xl 82.80% <83.33%> (+<0.01%) ⬆️
6.1-m7g.metal 80.15% <83.33%> (+<0.01%) ⬆️
6.1-m7i.metal-24xl 83.49% <83.33%> (+<0.01%) ⬆️
6.1-m7i.metal-48xl 83.49% <83.33%> (+<0.01%) ⬆️
6.1-m8g.metal-24xl 80.15% <83.33%> (+<0.01%) ⬆️
6.1-m8g.metal-48xl 80.15% <83.33%> (+<0.01%) ⬆️
6.1-m8i.metal-48xl 83.50% <83.33%> (+<0.01%) ⬆️
6.1-m8i.metal-96xl 83.49% <83.33%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Manciukic
Manciukic reviewed Feb 18, 2026
View reviewed changes
Copy link
Contributor

@Manciukic Manciukic left a comment

Choose a reason for hiding this comment

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

change lgtm. Let's add a changelog line as a fix, although minor.

Also the commit message is a little confusing:

- This change causes the error in metrics init code. Now if the
- metrics file is FIFO and Fircracker uses it as metrics file,
- the `open` will fail if there are no readers already reading from
- the FIFO. The error is `ENXIO` and is described in the `open` call
- man pages:
+ This causes an error during metrics and logs file initialization code if the
+ the file is a FIFO and there are no readers already reading from it.
+ This is because `open` returns `ENXIO` when opening a FIFO write-only as 
+ described in the man page:

ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 19, 2026
@ShadowCurse
changelog: add note about FIFO fix
4de1a0d 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 19, 2026
@ShadowCurse
changelog: add note about FIFO fix
3f77de2 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 19, 2026
@ShadowCurse
changelog: add note about FIFO fix
0b9f551 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 19, 2026
@ShadowCurse
changelog: add note about FIFO fix
3115855 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
JackThomson2
JackThomson2 previously approved these changes Feb 19, 2026
View reviewed changes
@ShadowCurse
fix: open files with read modifier
01821cf 
The commit
b656ae3
("fix: Support creating file with open_file_nonblock")
did modify the file opening utility function by adding
`open` option, but it also removed the `read` option from it.
This causes an error during metrics and logs file initialization code if
the the file is a FIFO and there are no readers already reading from it.
This is because `open` returns `ENXIO` when opening a FIFO write-only as
described in the man page:
```
ENXIO  O_NONBLOCK | O_WRONLY is set, the named file is a FIFO,  and  no
             process has the FIFO open for reading.
```
Fix is just a partial revert of the part that changed the file opening
logic by re-introducing same `open_file_nonblock` as it was before but
with added `create` flag.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 19, 2026
@ShadowCurse
changelog: add note about FIFO fix
885e374 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Manciukic
Manciukic reviewed Feb 19, 2026
View reviewed changes
@ShadowCurse
cleanup: remove redundant std::result::Result imports
f0bd845 
No functional change. Just cleanup.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse
changelog: add note about FIFO fix
8d28ab2 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@JackThomson2 JackThomson2 merged commit c07bff1 into firecracker-microvm:main Feb 20, 2026
7 checks passed
@ShadowCurse ShadowCurse deleted the save_fifos branch February 20, 2026 13:22
@ShadowCurse ShadowCurse restored the save_fifos branch February 20, 2026 14:53
@ShadowCurse ShadowCurse deleted the save_fifos branch February 20, 2026 14:53
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 20, 2026
@ShadowCurse
changelog: add note about FIFO fix
6201766 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 20, 2026
@ShadowCurse
changelog: add note about FIFO fix
83c2291 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse mentioned this pull request Feb 20, 2026
Merged
11 tasks
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 20, 2026
@ShadowCurse
changelog: add note about FIFO fix
5961906 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 20, 2026
@ShadowCurse
changelog: add note about FIFO fix
de49e71 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
JackThomson2 pushed a commit that referenced this pull request Feb 20, 2026
@ShadowCurse @JackThomson2
changelog: add note about FIFO fix
f7bd736 
Add note about fix in the #5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@ShadowCurse ShadowCurse mentioned this pull request Feb 20, 2026
Merged
11 tasks
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 23, 2026
@ShadowCurse
changelog: add note about FIFO fix
93b6f5e 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit to ShadowCurse/firecracker that referenced this pull request Feb 23, 2026
@ShadowCurse
changelog: add note about FIFO fix
4f86587 
Add note about fix in the firecracker-microvm#5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
ShadowCurse added a commit that referenced this pull request Feb 24, 2026
@ShadowCurse
changelog: add note about FIFO fix
3a12ebf 
Add note about fix in the #5698 PR.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Manciukic Manciukic Manciukic approved these changes

@JackThomson2 JackThomson2 JackThomson2 approved these changes

@xmarcalx xmarcalx Awaiting requested review from xmarcalx xmarcalx is a code owner

@kalyazin kalyazin Awaiting requested review from kalyazin kalyazin is a code owner

@pb8o pb8o Awaiting requested review from pb8o pb8o is a code owner

Assignees

@ShadowCurse ShadowCurse

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Fix Indicates a fix to existing code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ShadowCurse @Manciukic @JackThomson2