Fix all clippy warnings, add Github action check, and other minor cleanups #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Need to fix the build with the GDB stub it seems. |
|
Still need to fix warnings about |
|
Should be ready now. The clippy checks are now more strict, failing on warnings as well. |
00xc
force-pushed
the
clippy
branch
4 times, most recently
from
b8fc2ef
to
a59ee7d
Compare
|
Fixed some extra warnings I was not seeing due to having a slightly outdated nightly toolchain. Should be good to merge now. |
.github/workflows/rust.yml
Outdated
| uses: actions-rs/cargo@v1 | ||
| with: | ||
| command: clippy | ||
| args: --all-features -- -D warnings |
There was a problem hiding this comment.
What about adding --locked ?
In this way, we will have an error if Cargo.lock is not updated
There was a problem hiding this comment.
Hm, this would block new PRs if our dependencies are outdated, meaning that we would need to first update them manually, which is not ideal. We are looking into what's the best way of doing this, perhaps using something like Dependabot. For now we'll keep this PR as it is, and handle this item in a separate PR.
There was a problem hiding this comment.
I see, maybe we could have a warning, because otherwise the CI might compile something different from who posted the code. But I agree we can do later ;-)
Since these functions are only used when specific features are
enabled, allow them to be unused. Fixes two warnings like the
following:
warning: function `read_u8` is never used
--> src/mm/guestmem.rs:14:11
|
14 | pub unsafe fn read_u8(v: VirtAddr) -> Result<u8, SvsmError> {
| ^^^^^^^
|
= note: `#[warn(dead_code)]` on by default
Signed-off-by: Carlos López <carlos.lopez@suse.com>
SpinLock::try_lock() can either successfully grab the lock or not, so
return an Option, since we do not need any error information.
Fixes the following clippy warning:
warning: this returns a `Result<_, ()>`
--> src/locking/spinlock.rs:54:5
|
54 | pub fn try_lock(&self) -> Result<LockGuard<T>, ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: use a custom `Error` type instead
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err
= note: `#[warn(clippy::result_unit_err)]` on by default
Signed-off-by: Carlos López <carlos.lopez@suse.com>
If a pub method is associated with a private struct it will not be accessible anyway, so do not declare them as pub. Signed-off-by: Carlos López <carlos.lopez@suse.com>
Place together LockGuard impls for better readability Signed-off-by: Carlos López <carlos.lopez@suse.com>
Derive Debug in multiple structures to ease debugging and error reporting in tests. This has very little cost because unused derives should get optimized away during link time optimization. Signed-off-by: Carlos López <carlos.lopez@suse.com>
Replace multiple instances of statements in the form of `assert!(result.is_ok())` with `result.unwrap()`, since unwrap() will display the encountered error, and normally we are unwrapping a few lines after anyway. This simplifies the tests and improves error reporting if anything fails. Signed-off-by: Carlos López <carlos.lopez@suse.com>
Fixes the following clippy warning: warning: module has the same name as its containing module --> src/fs/mod.rs:8:1 | 8 | mod fs; | ^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_inception = note: `#[warn(clippy::module_inception)]` on by default Signed-off-by: Carlos López <carlos.lopez@suse.com>
RamDirectory has an internal lock protecting access to the directory
entries, so it can safely implement Send. This fixes the following
clippy warning:
error: usage of `Arc<T>` where `T` is not `Send` or `Sync`
--> src/fs/filesystem.rs:135:20
|
135 | let root_dir = Arc::new(RamDirectory::new());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider using `Rc<T>` instead or wrapping `T` in a std::sync type like `Mutex<T>`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
= note: `#[deny(clippy::arc_with_non_send_sync)]` on by default
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Fixes the following clippy warning:
warning: returning the result of a `let` binding from a block
--> src/debug/gdbstub.rs:158:13
|
157 | let res = unsafe { Ok(GDB_SERIAL.get_byte()) };
| ----------------------------------------------- unnecessary `let` binding
158 | res
| ^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
= note: `#[warn(clippy::let_and_return)]` on by default
help: return the expression directly
|
157 ~
158 ~ unsafe { Ok(GDB_SERIAL.get_byte()) }
|
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Replace range loops for iterators. Fixes the following clippy
warnings:
warning: the loop variable `offset` is used to index `data`
--> src/debug/gdbstub.rs:315:27
|
315 | for offset in 0..data.len() {
| ^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
= note: `#[warn(clippy::needless_range_loop)]` on by default
help: consider using an iterator and enumerate()
|
315 | for (offset, <item>) in data.iter_mut().enumerate() {
| ~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
warning: the loop variable `offset` is used to index `data`
--> src/debug/gdbstub.rs:333:27
|
333 | for offset in 0..data.len() {
| ^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
help: consider using an iterator and enumerate()
|
333 | for (offset, <item>) in data.iter().enumerate() {
|
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Fixes the following clippy warning:
warning: called `is_some()` after searching an `Iterator` with `find`
--> src/debug/gdbstub.rs:207:18
|
207 | .find(|&b| b.addr.bits() == rip)
| __________________^
208 | | .is_some()
| |__________________________^ help: use `any()` instead: `any(|b| b.addr.bits() == rip)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#search_is_some
= note: `#[warn(clippy::search_is_some)]` on by default
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Fixes the following clippy warning:
warning: unneeded `return` statement
--> src/debug/gdbstub.rs:220:13
|
220 | return unsafe { write_u8(guard.virt_addr() + phys.page_offset(), value) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
= note: `#[warn(clippy::needless_return)]` on by default
help: remove `return`
|
220 - return unsafe { write_u8(guard.virt_addr() + phys.page_offset(), value) };
220 + unsafe { write_u8(guard.virt_addr() + phys.page_offset(), value) }
|
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Remove a `map_err()` that was mistakenly introduced. Since the error is unwrapped via `expect()` just after, this was effectively doing nothing. Indirectly fixes the following warning: warning: casting integer literal to `u64` is unnecessary --> src/debug/gdbstub.rs:118:38 | 118 | .map_err(|_| 1 as u64) | ^^^^^^^^ help: try: `1_u64` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast = note: `#[warn(clippy::unnecessary_cast)]` on by default Signed-off-by: Carlos López <carlos.lopez@suse.com>
Get a mutable reference to the breakpoint we are modifying via iter_mut() + find() instead of retrieving the breakpoint's index. Signed-off-by: Carlos López <carlos.lopez@suse.com>
These functions correctly handle access faults to invalid addresses,
so they are safe to use.
Fixes the following clippy warnings:
warning: unsafe function's docs miss `# Safety` section
--> src/mm/guestmem.rs:15:1
|
15 | pub unsafe fn read_u8(v: VirtAddr) -> Result<u8, SvsmError> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
= note: `#[warn(clippy::missing_safety_doc)]` on by default
warning: unsafe function's docs miss `# Safety` section
--> src/mm/guestmem.rs:42:1
|
42 | pub unsafe fn write_u8(v: VirtAddr, val: u8) -> Result<(), SvsmError> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Since the SVSM now builds with no clippy warnings, add a GitHub Actions check for future PRs. Signed-off-by: Carlos López <carlos.lopez@suse.com>
We get the following warning on the macro-expanded RMPFlags struct:
error: &-masking with zero
--> src/sev/utils.rs:128:1
|
128 | / bitflags::bitflags! {
129 | | pub struct RMPFlags: u64 {
130 | | #[allow(clippy::bad_bit_mask)]
131 | | const VMPL0 = 0;
... |
144 | | }
145 | | }
| |_^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask
= note: this error originates in the macro `__impl_bitflags` which comes from the expansion of the macro `bitflags::bitflags` (in Nightly builds, run with -Z macro-backtrace for more info)
This happens because the VMPL0 flag is equal to the empty set of
flags. We want to keep this however because it improves readability.
Adding an individual #[allow(...)] statement on the structure does
not work due to macro expansion, so add an exception to the clippy
action.
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Signed-off-by: Carlos López <carlos.lopez@suse.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the remaining clippy warnings and add a GitHub action to run clippy on every PR.
This PR also adds a few minor cleanups like removing unneeded
pubs, adding missing derives and simplifying of some tests.