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

Fix some warnings and errors #39

Closed
wants to merge 2 commits into from
Closed

Conversation

alexandruag
Copy link
Contributor

@alexandruag alexandruag commented Jun 13, 2020

This fixes some warnings and errors such as:

  • We were implementing the now deprecated Error::description for various error types.
  • The Cargo.toml dev dependency to vm-memory did not refer a particular version.
  • The include_bytes!("bzimage") statement led to a build time error for doc and unit tests when the file did not exist.

Fixes #35
Related to #36

@alexandruag alexandruag force-pushed the fixes branch 3 times, most recently from 34e43dc to 962afa9 Compare June 13, 2020 20:30
alxiord
alxiord previously approved these changes Jun 15, 2020
@alxiord
Copy link
Member

alxiord commented Jun 15, 2020

#35

alxiord
alxiord previously approved these changes Jun 15, 2020
@andreeaflorescu
Copy link
Member

@alexandruag @aghecenco can you please add the text:
" Fixes: link/to/issue/" in the commit message of each commit. We have no way of tracing commits to PRs otherwise and when issues are available it's nice for tracing them back. Also, it automatically closes the issue when the commit is merged.

We should add this to some contributing guidelines as well that we can point people to. I'll open an issue in the community repository.

@alexandruag
Copy link
Contributor Author

alexandruag commented Jun 16, 2020

Makes sense. What's the approach when multiple commits are part of addressing the same issue? Do we say "fixes X"/"part of fixing X" for every one of them?

@andreeaflorescu
Copy link
Member

What I used so far is: "Related to #issue"

alexandruag and others added 2 commits June 17, 2020 12:28
Related to rust-vmm#35

Signed-off-by: Alexandru Agache <[email protected]>
Related to rust-vmm#36

Signed-off-by: Alexandra Iordache <[email protected]>
@alexandruag
Copy link
Contributor Author

What I used so far is: "Related to #issue"

Done, please take a look!

@andreeaflorescu
Copy link
Member

There are still the following warnings with newer clippy versions:

warning: long literal lacking separators
   --> src/configurator/x86_64/linux.rs:125:35
    |
125 |     const KERNEL_HDR_MAGIC: u32 = 0x53726448;
    |                                   ^^^^^^^^^^ help: consider: `0x5372_6448`
    |
    = note: `#[warn(clippy::unreadable_literal)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/configurator/x86_64/linux.rs:127:45
    |
127 |     const KERNEL_MIN_ALIGNMENT_BYTES: u32 = 0x1000000;
    |                                             ^^^^^^^^^ help: consider: `0x0100_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/configurator/x86_64/pvh.rs:167:44
    |
167 |     const XEN_HVM_START_MAGIC_VALUE: u32 = 0x336ec578;
    |                                            ^^^^^^^^^^ help: consider: `0x336e_c578`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/configurator/x86_64/pvh.rs:229:40
    |
229 |         let himem_start = GuestAddress(0x100000);
    |                                        ^^^^^^^^ help: consider: `0x0010_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/loader/x86_64/elf/mod.rs:378:40
    |
378 |         let kernel_addr = GuestAddress(0x200000);
    |                                        ^^^^^^^^ help: consider: `0x0020_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/loader/x86_64/elf/mod.rs:387:59
    |
387 |         assert_eq!(loader_result.kernel_load.raw_value(), 0x300000);
    |                                                           ^^^^^^^^ help: consider: `0x0030_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/loader/x86_64/elf/mod.rs:390:59
    |
390 |         assert_eq!(loader_result.kernel_load.raw_value(), 0x300000);
    |                                                           ^^^^^^^^ help: consider: `0x0030_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/loader/x86_64/elf/mod.rs:399:59
    |
399 |         assert_eq!(loader_result.kernel_load.raw_value(), 0x100000);
    |                                                           ^^^^^^^^ help: consider: `0x0010_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/loader/x86_64/elf/mod.rs:401:46
    |
401 |         highmem_start_address = GuestAddress(0xa00000);
    |                                              ^^^^^^^^ help: consider: `0x00a0_0000`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: long literal lacking separators
   --> src/loader/x86_64/elf/mod.rs:457:71
    |
457 |         assert_eq!(loader_result.pvh_entry_addr.unwrap().raw_value(), 0x1e1fe1f);
    |                                                                       ^^^^^^^^^ help: consider: `0x01e1_fe1f`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal

warning: use of a blacklisted/placeholder name `foo`
   --> src/configurator/mod.rs:514:13
    |
514 |         let foo = Foobar::default();
    |             ^^^
    |
    = note: `#[warn(clippy::blacklisted_name)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name

warning: casting a character literal to `u8` truncates
   --> src/loader/mod.rs:251:25
    |
251 |         assert_eq!(val, '1' as u8);
    |                         ^^^^^^^^^ help: use a byte literal instead: `b'1'`
    |
    = note: `#[warn(clippy::char_lit_as_u8)]` on by default
    = note: `char` is four bytes wide, but `u8` is a single byte
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8

warning: casting a character literal to `u8` truncates
   --> src/loader/mod.rs:254:25
    |
254 |         assert_eq!(val, '2' as u8);
    |                         ^^^^^^^^^ help: use a byte literal instead: `b'2'`
    |
    = note: `char` is four bytes wide, but `u8` is a single byte
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8

warning: casting a character literal to `u8` truncates
   --> src/loader/mod.rs:257:25
    |
257 |         assert_eq!(val, '3' as u8);
    |                         ^^^^^^^^^ help: use a byte literal instead: `b'3'`
    |
    = note: `char` is four bytes wide, but `u8` is a single byte
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8

warning: casting a character literal to `u8` truncates
   --> src/loader/mod.rs:260:25
    |
260 |         assert_eq!(val, '4' as u8);
    |                         ^^^^^^^^^ help: use a byte literal instead: `b'4'`
    |
    = note: `char` is four bytes wide, but `u8` is a single byte
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8

warning: casting a character literal to `u8` truncates
   --> src/loader/mod.rs:263:25
    |
263 |         assert_eq!(val, '\0' as u8);
    |                         ^^^^^^^^^^ help: use a byte literal instead: `b'\0'`
    |
    = note: `char` is four bytes wide, but `u8` is a single byte
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8

    Finished dev [unoptimized + debuginfo] target(s) in 12.55s

We can leave them open, but we shouldn't say it fixes #36.

@alxiord
Copy link
Member

alxiord commented Jun 17, 2020

There are still the following warnings with newer clippy versions:

Which version?

@andreeaflorescu
Copy link
Member

There are still the following warnings with newer clippy versions:

Which version?

1.43.1 (the one specified in the issue #36).

@andreeaflorescu
Copy link
Member

After a chat with @alexandruag I realized I was running clippy differently :)))

Specifically, I was also running it for the tests. Actual command I was using:
cargo clippy --all --all-targets

The rust-vmm-ci is running clippy with:
cargo clippy --all -- -D warnings

We should check if it is running it for all features though.

@alexandruag
Copy link
Contributor Author

alexandruag commented Jun 18, 2020

So I had a look at what the various clippy parameters do (clippy -h does not seem super helpful at first, but then I noticed it mentions most option are the same as for cargo check), and it looks like we should run something like cargo clippy --workspace --bins --examples --tests --benches --all-targets --all-features -- -D warnings to cover everything.

Some options will be redundant most of the time, but at the same time they don't do any harm. Also, --all-features might not be valid for some crates, but we'll figure this out on a case by case basis. A weird thing is that running with the 1.44.0 (current stable) toolchain leads to less warnings, and the -- -D warnings does not seem to take effect.

Created rust-vmm/rust-vmm-ci#21.

@andreeaflorescu
Copy link
Member

Closing this due to lack of activity and many conflicts.

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.

Replace std::error::description with to_string()
3 participants