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.

Sign up for GitHub

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

Jump to bottom

Refactoring around daemon launch #389

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Refactoring around daemon launch #389

merged 2 commits into from
Nov 20, 2023

Conversation

Ablu
Copy link
Contributor

@Ablu Ablu commented Jul 7, 2023 •
edited
Loading

Summary of the PR

Depends on rust-vmm/vhost#173

This cleans up a couple of error reporting related issues and introduces use
of the proposed serve API [1].

[1] rust-vmm/vhost#173

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (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.

@Ablu Ablu mentioned this pull request Jul 7, 2023
Merged
4 tasks
Ablu added a commit to Ablu/vhost-device that referenced this pull request Jul 10, 2023
@Ablu
tree-wide: bubble up errors from daemon threads
c71d446 
There was a mix of just unwrapping (panicking) and catching and logging
errors. The unwrapping is not allowing for particulary pretty error
handling, so let's bubble the errors up by not crashing the thread,
but by just returning a Result<()> than is received when joining the
threads.

Not all .unwrap() uses were translated since a followup PR (rust-vmm#389) will
rework that code anyway (and get rid of the .unwrap() in the process).

Signed-off-by: Erik Schilling <[email protected]>
@Ablu Ablu mentioned this pull request Jul 10, 2023
Merged
4 tasks
Ablu added a commit to Ablu/vhost-device that referenced this pull request Jul 10, 2023
@Ablu
tree-wide: bubble up errors from daemon threads
d5d930a 
There was a mix of just unwrapping (panicking) and catching and logging
errors. The unwrapping is not allowing for particulary pretty error
handling, so let's bubble the errors up by not crashing the thread,
but by just returning a Result<()> than is received when joining the
threads.

Not all .unwrap() uses were translated since a followup PR (rust-vmm#389) will
rework that code anyway (and get rid of the .unwrap() in the process).

Signed-off-by: Erik Schilling <[email protected]>
Ablu added a commit to Ablu/vhost-device that referenced this pull request Jul 10, 2023
@Ablu
tree-wide: bubble up errors from daemon threads
1aec3aa 
There was a mix of just unwrapping (panicking) and catching and logging
errors. The unwrapping is not allowing for particulary pretty error
handling, so let's bubble the errors up by not crashing the thread,
but by just returning a Result<()> than is received when joining the
threads.

Not all .unwrap() uses were translated since a followup PR (rust-vmm#389) will
rework that code anyway (and get rid of the .unwrap() in the process).

Signed-off-by: Erik Schilling <[email protected]>
vireshk pushed a commit that referenced this pull request Jul 11, 2023
@Ablu @vireshk
tree-wide: bubble up errors from daemon threads
a9de87a 
There was a mix of just unwrapping (panicking) and catching and logging
errors. The unwrapping is not allowing for particulary pretty error
handling, so let's bubble the errors up by not crashing the thread,
but by just returning a Result<()> than is received when joining the
threads.

Not all .unwrap() uses were translated since a followup PR (#389) will
rework that code anyway (and get rid of the .unwrap() in the process).

Signed-off-by: Erik Schilling <[email protected]>
@Ablu
Copy link
Contributor Author

Ablu commented Jul 11, 2023

Changelog

@Ablu
Copy link
Contributor Author

Ablu commented Sep 14, 2023

The dependencies for this landed now. Shall we do a new release of the vhost-user-backend crate or just point vhost-device's dependencies at a git snapshot until a release is happening naturally?

@stefano-garzarella
Copy link
Member

The dependencies for this landed now. Shall we do a new release of the vhost-user-backend crate or just point vhost-device's dependencies at a git snapshot until a release is happening naturally?

I'd suggest rebasing it, keep the git snapshot (referring to the rust-vmm/vhost repo), and when we will have the release ready, update the reference and make this PR as ready.

In the end, I'd merge it only after releasing vhost-user-backend. (Maybe next week we can think about releasing it if we don't have any pending PR to finish)

@Ablu
Copy link
Contributor Author

Ablu commented Sep 18, 2023

The dependencies for this landed now. Shall we do a new release of the vhost-user-backend crate or just point vhost-device's dependencies at a git snapshot until a release is happening naturally?

I'd suggest rebasing it, keep the git snapshot (referring to the rust-vmm/vhost repo), and when we will have the release ready, update the reference and make this PR as ready.

In the end, I'd merge it only after releasing vhost-user-backend. (Maybe next week we can think about releasing it if we don't have any pending PR to finish)

ACK. Though, since it would need an update to drop the git dependency reference anyway, I think I will just wait for the release to happen and then rebase this.

@Ablu Ablu marked this pull request as ready for review November 14, 2023 13:29
@Ablu Ablu changed the title DRAFT: Refactoring around daemon launch Refactoring around daemon launch Nov 14, 2023
stefano-garzarella
stefano-garzarella reviewed Nov 16, 2023
View reviewed changes
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.

I mostly reviewed sound and vsock since I know them better, but overall LGTM!

I left a minor comment and a question that eventually should be fixed in another PR.

staging/vhost-device-sound/src/lib.rs Outdated Show resolved Hide resolved
vhost-device-gpio/src/backend.rs Show resolved Hide resolved
epilys
epilys previously requested changes Nov 16, 2023
View reviewed changes
vhost-device-gpio/src/backend.rs Show resolved Hide resolved
@Ablu Ablu mentioned this pull request Nov 16, 2023
Open
@Ablu
Copy link
Contributor Author

Ablu commented Nov 16, 2023

Changelog

  • changed manual clone to calling of socket_path getter

@stefano-garzarella
Copy link
Member

The CI is complaining about cargo-audit report. I already asked dependabot to check new updates, so we should have a PR soon that will fix it.

@epilys epilys dismissed their stale review November 17, 2023 15:33

Resolved

@Ablu
Copy link
Contributor Author

Ablu commented Nov 17, 2023

Changelog:

  • tweaked error message

Ablu added 2 commits November 20, 2023 08:27
@Ablu
tree-wide: use new serve convenience function
f0c26dd 
This become available with the recent vhost-user-backend [1] updates and
allows to get rid of some boilerplate code.

[1] rust-vmm/vhost#173

Signed-off-by: Erik Schilling <[email protected]>
@Ablu
tree-wide: continue unwinds on join failures
2fb4b6a 
The error from joining a thread is a bit confusing. It is only printed
if the other thread panicked. This means, effectively, we only get here
if something called .unwrap(), .expect() or panicked in a different way.
In these cases an (ugly) error was already printend. Printing a pretty
message about the join failure does not really help a lot...

So let's just continue the unwind as suggested by the docs on the join
`Result` [1].

Before:

    thread '<unnamed>' panicked at 'Test panic', crates/gpio/src/backend.rs:146:13
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
    thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any { .. }', crates/gpio/src/backend.rs:176:23

After:

    thread '<unnamed>' panicked at 'Test panic', crates/gpio/src/backend.rs:146:13
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[1]: https://doc.rust-lang.org/std/thread/type.Result.html

Signed-off-by: Erik Schilling <[email protected]>
@Ablu
Copy link
Contributor Author

Ablu commented Nov 20, 2023

rebased in the hope that recent updates fixed the yanked cc

@vireshk
Copy link
Collaborator

vireshk commented Nov 20, 2023

Finally the redundant code is removed. Yay :)

@vireshk vireshk merged commit 2143bcb into rust-vmm:main Nov 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vireshk vireshk vireshk approved these changes

@epilys epilys epilys approved these changes

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

@stefano-garzarella stefano-garzarella Awaiting requested review from stefano-garzarella stefano-garzarella is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ablu @stefano-garzarella @vireshk @epilys