tree-wide: improve and refactor error printing #388
Conversation
Ablu
requested review from
mathieupoirier,
stsquad,
vireshk and
stefano-garzarella
as code owners
|
Does this need a CHANGELOG.md entry? It is certainly public-facing, but it does not change anything implementation wise. |
Changelog
|
|
I realized that I may have to do a similar update to vsock too. But merging this does not hurt. Will include the vsock fixes in a new PR. |
coverage_config_x86_64.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "coverage_score": 69.6, | |||
| "coverage_score": 69.0, | |||
There was a problem hiding this comment.
Is coverage dropping because the tests aren't exercising the error exit path?
There was a problem hiding this comment.
Yes.
It must have been right on the corner of the allowed interval. Tests are likely just starting on the start_foo level since they still want the Results. Of course one could try to write tests for this. But I am not sure how valuable those would be (they would also have to re-test major chunks of what others tests are already testing). I am also not sure how well it would play with the exit(1) call...
We setup pretty-printing with all the thiserror-based Error structs, but
then only bubble the error up to the main, where it just gets printed
with the `Debug` trait.
This "catches" the error in the main and performs pretty printing using
the `Display` trait.
Before:
Error: FailedCreatingListener(SocketError(Os { code: 98, kind:
AddrInUse, message: "Address already in use" }))
After:
[2023-07-06T17:20:47Z ERROR vhost_device_scsi] Failed creating
listener: socket error: Address already in use (os error 98)
vhost-device-vsock is a bit special since it does not let error messages
bubble up to the main. It also does .unwrap() in most places, but it
_does_ pretty print errors during the main request handling part.
Had to slightly adjust the coverage since we have no tests for the main
functions.
Signed-off-by: Erik Schilling <[email protected]>
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Changelog
- Added some related commits from Refactoring around daemon launch #389
It looks like the error handling is not properly tested, which now shows in degraded code coverage. I will see whether I can add some tests to cover that.
After looking into this: It is not as easy... Testing these corner-cases would need to deal with actual gpio / i2c devices. So we would need to enable the kernel mocking mechanisms for this. So I think I would just consider the ~1% coverage drop as coverage that was already missing previously (for |
Ablu
changed the title
I plan to add some wrapper errors around vhost errors. These end up nesting other errors all the way to std::error::Error, which has no Eq trait. The implementations were only used for comparisions in tests. While there is a assert_matches!() in nightly [1] it seems unlikely that further testing lib additions are getting standarized soon (or ever). One could use assert!(matches!()), but that would worsen the error messages for test failures. Hence, during review [2] we agreed on introducing the assert_matches crate. It got no dependencies and allows us to keep the good error messages while not needing to depend on nightly. [1] https://doc.rust-lang.org/std/assert_matches/macro.assert_matches.html [2] rust-vmm#388 (comment) Signed-off-by: Erik Schilling <[email protected]>
Changelog
|
I plan to add some wrapper errors around vhost errors. These end up nesting other errors all the way to std::error::Error, which has no Eq trait. The implementations were only used for comparisions in tests. While there is a assert_matches!() in nightly [1] it seems unlikely that further testing lib additions are getting standarized soon (or ever). One could use assert!(matches!()), but that would worsen the error messages for test failures. Hence, during review [2] we agreed on introducing the assert_matches crate. It got no dependencies and allows us to keep the good error messages while not needing to depend on nightly. Signed-off-by: Erik Schilling <[email protected]> [1] https://doc.rust-lang.org/std/assert_matches/macro.assert_matches.html [2] rust-vmm#388 (comment)
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]>
If the parsed commandline options cannot be converted into a `VuRngConfig` we would only get a .unwrap() error message. Chain this before the check of the daemon launch. Suggested-by: Manos Pitsidianakis <[email protected]> Signed-off-by: Erik Schilling <[email protected]>
|
Fixed the commit log linter by moving things after the Sign-Off again... I really want rust-vmm/rust-vmm-ci#130 ... |
|
@stefano-garzarella can you approve this too ? |
I plan to add some wrapper errors around vhost errors. These end up nesting other errors all the way to std::error::Error, which has no Eq trait. The implementations were only used for comparisions in tests. While there is a assert_matches!() in nightly [1] it seems unlikely that further testing lib additions are getting standarized soon (or ever). One could use assert!(matches!()), but that would worsen the error messages for test failures. Hence, during review [2] we agreed on introducing the assert_matches crate. It got no dependencies and allows us to keep the good error messages while not needing to depend on nightly. Signed-off-by: Erik Schilling <[email protected]> [1] https://doc.rust-lang.org/std/assert_matches/macro.assert_matches.html [2] #388 (comment)
This PR groups some patches that improve the error printing and reporting mechanisms.
Overall, this reduces the number of .unwrap() calls used and starts to actually
put the pretty error messages that we already have defined into use.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.