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

Check for correctness of feature argument number when parsing feature arguments #556

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

waltdisgrace
Copy link
Contributor

Preliminary PR for #485

@jbaublitz
Copy link
Member

@waltdisgrace Ping me here once you've figured out the testing issues, happy to help if you get stuck.

src/lineardev.rs Outdated Show resolved Hide resolved
@mulkieran mulkieran changed the title Check for correctness of user-inputted feature argument number Check for correctness of feature argument number when parsing feature argumens Jun 29, 2020
@mulkieran mulkieran changed the title Check for correctness of feature argument number when parsing feature argumens Check for correctness of feature argument number when parsing feature arguments Jun 29, 2020
@waltdisgrace
Copy link
Contributor Author

Rebased.

@mulkieran
Copy link
Member

mulkieran commented Jul 8, 2020

@waltdisgrace Please post the error that you obtain from your newly added test with the unmodified code, i.e., the code w/out your additional changes in the source.

@waltdisgrace
Copy link
Contributor Author

waltdisgrace commented Jul 9, 2020

@mulkieran:

Here is the relevant snippet of the make test output:

failures:

---- lineardev::tests::test_flakey_incorrect_feature_args_input stdout ----
thread 'lineardev::tests::test_flakey_incorrect_feature_args_input' panicked at 'index 9 out of range for slice of length 8', src/lineardev.rs:303:18
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::io::impls::<impl std::io::Write for alloc::boxed::Box<W>>::write_fmt
   4: std::panicking::default_hook::{{closure}}
   5: std::panicking::default_hook
   6: std::panicking::rust_panic_with_hook
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::slice::slice_index_len_fail
  10: <core::ops::range::Range<usize> as core::slice::SliceIndex<[T]>>::index
             at /builddir/build/BUILD/rustc-1.44.0-src/src/libcore/slice/mod.rs:2919
  11: core::slice::<impl core::ops::index::Index<I> for [T]>::index
             at /builddir/build/BUILD/rustc-1.44.0-src/src/libcore/slice/mod.rs:2732
  12: <alloc::vec::Vec<T> as core::ops::index::Index<I>>::index
             at /builddir/build/BUILD/rustc-1.44.0-src/src/liballoc/vec.rs:1945
  13: <devicemapper::lineardev::FlakeyTargetParams as core::str::FromStr>::from_str
             at src/lineardev.rs:303
  14: core::str::<impl str>::parse
             at /builddir/build/BUILD/rustc-1.44.0-src/src/libcore/str/mod.rs:4280
  15: devicemapper::lineardev::tests::test_flakey_incorrect_feature_args_input
             at src/lineardev.rs:923
  16: devicemapper::lineardev::tests::test_flakey_incorrect_feature_args_input::{{closure}}
             at src/lineardev.rs:922
  17: core::ops::function::FnOnce::call_once
             at /builddir/build/BUILD/rustc-1.44.0-src/src/libcore/ops/function.rs:232
  18: test::run_test::run_test_inner::{{closure}}
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@mulkieran
Copy link
Member

@waltdisgrace: Ok! That failure looks right to me.

src/lineardev.rs Outdated Show resolved Hide resolved
src/lineardev.rs Outdated Show resolved Hide resolved
@waltdisgrace
Copy link
Contributor Author

Rebased

Copy link
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation for Flakey looks reasonable to me. Linear won't have any feature args, because it is so simple, but I think all the other targets that are supported have feature args and will therefore each implementation will contain the same bug.

Please go ahead and pick another, e.g., ThinPoolDev and apply the same fix.

@waltdisgrace
Copy link
Contributor Author

@mulkieran Should the fixes for the other targets be done in separate PRs or the same PR as Flakey?

@mulkieran mulkieran assigned mulkieran and unassigned waltdisgrace Nov 25, 2020
@mulkieran
Copy link
Member

Unlikely to manage to get to this before next release.

@mulkieran mulkieran marked this pull request as draft March 10, 2021 13:40
@jbaublitz jbaublitz removed their request for review April 26, 2021 16:01
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.

3 participants