Check for correctness of feature argument number when parsing feature arguments

[waltdisgrace]

Preliminary PR for #485

[jbaublitz]

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

[waltdisgrace]

Rebased.

[mulkieran]

@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]

@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]

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

[waltdisgrace]

Rebased

[mulkieran]

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]

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

[mulkieran]

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

@waltdisgrace You should make up a separate commit in this PR for ThinPoolDev and for each of the other target types that require the change.

[waltdisgrace]

Rebased

[mulkieran]

One request. There's a complication w/ cache that did not exist w/ flakey because the feature args are followed by the policy field, which as far as I can tell is mandatory. But the code is still vulnerable to a panic with something like

+    #[test]
+    fn test_cache_target_params_too_few() {
+        let result = "cache 42:42 42:43 42:44 16 2 writethrough passthrough default 0"
+            .parse::<CacheTargetParams>();
+        assert_matches!(result, Err(DmError::Dm(ErrorEnum::Invalid, _)));
+    }

[waltdisgrace]

I pushed a commit containing all of the changes you explicitly requested.

Regarding the test you provided: This test causes a panic at the assertion that the input results in an error, not at the source code. The name you chose for the test, "... too_few", implies that the input, "cache 42:42 42:43 42:44 16 2 writethrough passthrough default 0", has too few arguments. However, this is a valid input that has enough arguments. Am I missing something?

[mulkieran]

I pushed a commit containing all of the changes you explicitly requested.

Regarding the test you provided: This test causes a panic at the assertion that the input results in an error, not at the source code. The name you chose for the test, "... too_few", implies that the input, "cache 42:42 42:43 42:44 16 2 writethrough passthrough default 0", has too few arguments. However, this is a valid input that has enough arguments. Am I missing something?

Whoops! Yes, I had modified the test to try out some other things. Try this instead:

    #[test]
    fn test_cache_target_params_too_few() {
        let result = "cache 42:42 42:43 42:44 16 4 writethrough passthrough default 0"
            .parse::<CacheTargetParams>();
        assert_matches!(result, Err(DmError::Dm(ErrorEnum::Invalid, _)));
    }

[waltdisgrace]

@mulkieran. That test looks reasonable to me and it passes when run against the source code changes of my most recently pushed commit. Adding your test to the PR now.

[mulkieran]

A few more requests...I'll probably need one more pass.

[mulkieran]

Just make this if vals.len() < end_feature_args_index and then make the error message specific to too few feature_args. That change means we could still fail on policy missing, but I want to leave that until later, also.

[waltdisgrace]

To avoid an out-of-bounds panic on test inputs that are only incorrect because of an incorrect feature argument number, this condition was chosen. Changing the condition to if vals.len() < end_feature_args_index would make sense for test inputs that are incorrect for the additional reason of being void of mandatory policy args, e.g. "cache 42:42 42:43 42:44 16 4 writethrough".

[mulkieran]

Unlikely to manage to get to this before next release.