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

Update vhost-device-console backend & move it in main workspace #727

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TimosAmpel
Copy link
Contributor

Summary of the PR

This PR updates the current implementation of vhost-device-console and addresses
the comments noted at the PR #717.

Specifically,

  • Eliminates the use of 'select' function and 'nix' dependency
  • Registers all input FDs to the main worker epoll handler
  • Increases the unit tests' coverage
  • Move the device into the main workspace

Requirements

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@stefano-garzarella
Copy link
Member

Summary of the PR

This PR updates the current implementation of vhost-device-console and addresses the comments noted at the PR #717.

Specifically,

* Eliminates the use of 'select' function and 'nix' dependency

Cool, thanks for that!

* Registers all input FDs to the main worker epoll handler

* Increases the unit tests' coverage

* Move the device into the main workspace

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.

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>
@TimosAmpel
Copy link
Contributor Author

Summary of the PR

This PR updates the current implementation of vhost-device-console and addresses the comments noted at the PR #717.
Specifically,

* Eliminates the use of 'select' function and 'nix' dependency

Cool, thanks for that!

* Registers all input FDs to the main worker epoll handler

* Increases the unit tests' coverage

* Move the device into the main workspace

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.

Thanks for the comment! I did split it into three difference commits, I believe that should be ok.

Copy link
Member

@stefano-garzarella stefano-garzarella left a 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]

Comment on lines -23 to -24
- [ ] Support VIRTIO_CONSOLE_F_SIZE feature (optional)
- [ ] Support VIRTIO_CONSOLE_F_EMERG_WRITE feature (optional)
Copy link
Member

Choose a reason for hiding this comment

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

Are we supporting these?

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.

2 participants