-
Notifications
You must be signed in to change notification settings - Fork 47
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
Update vhost-device-console backend & move it in main workspace #727
base: main
Are you sure you want to change the base?
Update vhost-device-console backend & move it in main workspace #727
Conversation
Cool, thanks for that!
All these steps in one commit? This list should immediately make it clear that we need at least 3/4 separate commits. Please let's try to have commits as small as possible, also to make it easier for reviewers. |
ffb9934
to
9bf3173
Compare
Eliminate the use of select and 'nix' package. This is done by registering the input events (stdin or tcplistener) onto the main worker's epoll. Signed-off-by: Timos Ampelikiotis <t.ampelikiotis@virtualopensystems.com>
Signed-off-by: Timos Ampelikiotis <t.ampelikiotis@virtualopensystems.com>
Signed-off-by: Timos Ampelikiotis <t.ampelikiotis@virtualopensystems.com>
9bf3173
to
a35f354
Compare
Thanks for the comment! I did split it into three difference commits, I believe that should be ok. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment! I did split it into three difference commits, I believe that should be ok.
@TimosAmpel thanks for that!
We need to adjust coverage value on both main work space and staging workspace to make CI happy.
Also I suggest better commit title/description, here some suggestions:
- patch 1
vhost-device-console: use worker's epoll for input events
Eliminate the use of select and 'nix' package. This is done by
registering the input events (stdin or tcplistener) onto the main
worker's epoll.
- patch 2
vhost-device-console: improve tests
[add a little descriptions of the new tests]
- patch 3
vhost-device-console: promote to main workspace
The current implementation seems ready to be promoted to the
main workspace since the device supports .... [add what we support, and what is still missing]
- [ ] Support VIRTIO_CONSOLE_F_SIZE feature (optional) | ||
- [ ] Support VIRTIO_CONSOLE_F_EMERG_WRITE feature (optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we supporting these?
Summary of the PR
This PR updates the current implementation of vhost-device-console and addresses
the comments noted at the PR #717.
Specifically,
Requirements
git commit -s
), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.