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

TimosAmpel commented 1 month ago

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

[x] 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.
[x] All added/changed functionality has a corresponding unit/integration test.
[x] All added/changed public-facing functionality has entries in the "Upcoming Release" section of CHANGELOG.md (if no such section exists, please create one).
[x] Any newly added unsafe code is properly documented.

stefano-garzarella commented 1 month ago

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.

TimosAmpel commented 1 month ago

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.

TimosAmpel commented 2 weeks ago

Hello @stefano-garzarella, thanks for your comments and sorry for my delayed answer. In the last push I have updated the commit messages based on what you requested and also modified the coverage values for both main and staging workspaces.

Since in the last commit vhost-device-console was move to main I reverted the coverage value of the staging to what it was before that patch serie.

Let me know if you have more comments regarding those updates.

stefano-garzarella commented 2 weeks ago

@TimosAmpel I just noticed that Cargo.lock need to be updated on the main workspace and staging, so maybe you can do that in 3rd patch.

TimosAmpel commented 2 weeks ago

@TimosAmpel great job with the coverage!

I tested it and I did a quick review. Just 1 comment, more curiosity, but in general LGTM, we can eventually improve the code later, but the main features are there.

Thanks for your work!

Thanks a lot for the review too!

TimosAmpel commented 2 weeks ago

@TimosAmpel I just noticed that Cargo.lock need to be updated on the main workspace and staging, so maybe you can do that in 3rd patch.

The Cargo.lock files for both main and staging have been updated in 3rd patch (also the coverage under staging needed a bit of tuning, I see the CI is satisfied now)

stefano-garzarella commented 2 weeks ago

Just as a reminder, when we merge this we need to do the following steps:

[ ] publish the first version on crates.io
[ ] create a tag and a release in this repo
[ ] unignore nix in dependabot and close #738

TimosAmpel commented 2 weeks ago

Just as a reminder, when we merge this we need to do the following steps:

[ ] publish the first version on crates.io

[ ] create a tag and a release in this repo

[ ] unignore nix in dependabot and close dependabot: unignore nix #738

Very nice! Thanks for the review @stefano-garzarella, we'll do as soon as this is merged

epilys commented 2 weeks ago

@stefano-garzarella btw all of us are at an employee meeting this week, we might have time from Saturday onwards.

stefano-garzarella commented 2 weeks ago

@stefano-garzarella btw all of us are at an employee meeting this week, we might have time from Saturday onwards.

@epilys thanks for the update! Take your time, we don't need to rush ;-)

TimosAmpel commented 2 weeks ago

Looks good from here too.

Thanks for your feedback @stsquad.

TimosAmpel commented 1 week ago

Just as a reminder, when we merge this we need to do the following steps:

[ ] publish the first version on crates.io

[ ] create a tag and a release in this repo

[ ] unignore nix in dependabot and close dependabot: unignore nix #738

Hello @garzarella and @epilys, Since both console and CAN device have been moved on the main workspace, do you think that we could also complete the above points? (mainly the crates.io and the creation of the tag/release)

epilys commented 1 week ago

@TimosAmpel certainly, the usual workflow is to make an issue and a maintainer will assign themselves to make the release. I will make an issue myself in a moment

TimosAmpel commented 1 week ago

@TimosAmpel certainly, the usual workflow is to make an issue and a maintainer will assign themselves to make the release. I will make an issue myself in a moment

Thanks @epilys, I will do so the next time :)

rust-vmm / vhost-device