Improve command-line arguments #1862

edigaryev · 2020-05-04T15:59:10Z

Reason for This PR

#1688

Description of Changes

#1688, with the following additions:

documented --start-time-us and --start-time-cpu-us, fixed start-time-cpu-us typo
one unit test for jailed (Unit test the jailer #363) to counter imprecise kcov results preventing coverage test to pass
- the most relevant issue for this behavior I've found is Rustc adds line-number information for unhittable panic handlers rust-lang/rust#55352

...and with the exception for:

Segregate mandatory and optional arguments properly.

See #1688 (comment).

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.

edigaryev · 2020-05-04T16:23:56Z

It seems that this got collided with #1861. What a time to be alive!

I can rebase the missing pieces if needed.

Also, it seems that this is getting hit by #1834, because tools/devtool test -- -k build works just fine.

shioyama18 · 2020-05-05T04:31:10Z

I'm sorry for the collision. I really liked your idea about using BTreeMap so I incorporated it in my commit. It made my code much simpler.
8324838

ioanachirca

Hello @edigaryev! Thanks for the pull request 😄
While some of the functionality that is in this PR was merged with #1861, we can keep the test for mknod_and_own_dev if you want to rebase this commit on top of the current changes.

src/jailer/src/env.rs

edigaryev · 2020-05-27T20:47:13Z

@ioanachirca should I bump the COVERAGE_TARGET_PCT?

lauralt · 2020-05-29T09:53:26Z

src/jailer/src/env.rs

+        let good_arg_vals = ArgVals {
+            node: "1",
+            id: "bd65600d-8669-4903-8a14-af88203add38",
+            exec_file: "/proc/cpuinfo",
+            uid: "1001",
+            gid: "1002",
+            chroot_base: "/",
+            netns: Some("zzzns"),
+            daemonize: true,
+        };


This can be replaced with let good_arg_vals = ArgVals::new();

See 1847636.

lauralt · 2020-05-29T10:51:29Z

src/jailer/src/env.rs

+        // Ensure TUN device node is created with correct major/minor numbers.
+        let tun = b"/dev/net/tun-test\0";
+        let tun_str = CStr::from_bytes_with_nul(tun).unwrap().to_str().unwrap();
+        fs::remove_file(tun_str).or_else(skip_not_found).unwrap();
+        env.mknod_and_own_dev(tun, DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR)
+            .unwrap();
+
+        let metadata = File::open(tun_str).unwrap().metadata().unwrap();
+        assert_eq!(metadata.file_type().is_char_device(), true);
+        assert_eq!(get_major(metadata.rdev()), DEV_NET_TUN_MAJOR);
+        assert_eq!(get_minor(metadata.rdev()), DEV_NET_TUN_MINOR);
+        assert_eq!(metadata.permissions().mode(), expected_device_permissions);


This part and the next one look very similar, only the device and its minor and major numbers differ. You can reduce the duplicate code by using a for, something like this:

let tun = b"/dev/net/tun-test\0"; let kvm = b"/dev/kvm-test\0"; let dev_infos: Vec<(&[u8], u32, u32)> = vec![ (tun, DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR), (kvm, DEV_KVM_MAJOR, DEV_KVM_MINOR), ]; for (dev, major, minor) in dev_infos { let dev_str = CStr::from_bytes_with_nul(dev).unwrap().to_str().unwrap(); ... }

What do you think?

Thanks! This looks way better. See 1847636.

src/jailer/src/env.rs

lauralt · 2020-05-29T11:24:18Z

Hi, @edigaryev! The coverage should be indeed updated with the new values, I suggest you to wait for the CI to finish in order to see also the coverage value for AMD.
Also, please can you squash your commits into a single one? There is no need to have a separate commit for minor changes like the ArgVals instantiation update or other small changes that we requested in review.
It would be awesome if you could adjust your commit title to follow the 50/72 commit rule.

edigaryev · 2020-05-29T12:14:13Z

Also, please can you squash your commits into a single one? There is no need to have a separate commit for minor changes like the ArgVals instantiation update or other small changes that we requested in review.

Squashed into 1847636.

It would be awesome if you could adjust your commit title to follow the 50/72 commit rule.

Applied 50/72 to 1847636.

I was initially trying to replicate the convention from git log, but that rule wasn't that easy to infer.

Perhaps the Contribution Quality Standards can be updated to reflect this practice?

lauralt

Looks good! I have two more nits.

Also, please don't forget to update the coverage target value in test_coverage.py.
And one more thing: there is no need to add me and Ioana as co-authors to your commit; you can remove those lines. It's your work and we're here to help you :D.

src/jailer/src/env.rs

lauralt · 2020-05-29T12:58:25Z

Also, please can you squash your commits into a single one? There is no need to have a separate commit for minor changes like the ArgVals instantiation update or other small changes that we requested in review.

Squashed into 1847636.

It would be awesome if you could adjust your commit title to follow the 50/72 commit rule.

Applied 50/72 to 1847636.

I was initially trying to replicate the convention from git log, but that rule wasn't that easy to infer.

Perhaps the Contribution Quality Standards can be updated to reflect this practice?

You mean we should add the 50/72 rule explicitly there? It is indeed pretty hidden here.

src/jailer/src/env.rs

edigaryev · 2020-05-29T13:49:25Z

You mean we should add the 50/72 rule explicitly there? It is indeed pretty hidden here.

Thanks, totally missed that.

Also, the Buildkite runner seems to be stuck on git clone stage with the "permission denied" error, no matter how many times it gets restarted: https://buildkite.com/firecracker/firecracker-ci-amd/builds/120

src/jailer/src/env.rs

lauralt · 2020-05-29T14:06:35Z

You mean we should add the 50/72 rule explicitly there? It is indeed pretty hidden here.

Thanks, totally missed that.

Also, the Buildkite runner seems to be stuck on git clone stage with the "permission denied" error, no matter how many times it gets restarted: https://buildkite.com/firecracker/firecracker-ci-amd/builds/120

Will fix it soon.

lauralt · 2020-05-29T14:40:16Z

@edigaryev Can you please update AMD coverage too and we will re-approve after that?

The test ensures that Env.mknod_and_own_dev() creates character devices with correct major/minor numbers and permissions. Signed-off-by: Nikolay Edigaryev <edigaryev@gmail.com>

edigaryev changed the title ~~Improve command-line arguments (#1688)~~ Improve command-line arguments May 4, 2020

lauralt added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label May 22, 2020

ioanachirca reviewed May 25, 2020

View reviewed changes

src/jailer/src/env.rs Show resolved Hide resolved

src/jailer/src/env.rs Outdated Show resolved Hide resolved

src/jailer/src/env.rs Outdated Show resolved Hide resolved

ioanachirca added Status: Author and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels May 26, 2020

edigaryev requested a review from ioanachirca May 28, 2020 17:18

lauralt self-requested a review May 29, 2020 09:09

lauralt reviewed May 29, 2020

View reviewed changes

lauralt assigned edigaryev May 29, 2020

edigaryev requested a review from lauralt May 29, 2020 12:15

lauralt reviewed May 29, 2020

View reviewed changes

src/jailer/src/env.rs Outdated Show resolved Hide resolved

src/jailer/src/env.rs Outdated Show resolved Hide resolved

lauralt reviewed May 29, 2020

View reviewed changes

src/jailer/src/env.rs Outdated Show resolved Hide resolved

edigaryev requested a review from lauralt May 29, 2020 13:56

ioanachirca reviewed May 29, 2020

View reviewed changes

src/jailer/src/env.rs Show resolved Hide resolved

src/jailer/src/env.rs Show resolved Hide resolved

lauralt previously approved these changes May 29, 2020

View reviewed changes

ioanachirca previously approved these changes May 29, 2020

View reviewed changes

jailer: add unit test for mknod_and_own_dev()

e11bd9b

The test ensures that Env.mknod_and_own_dev() creates character devices with correct major/minor numbers and permissions. Signed-off-by: Nikolay Edigaryev <edigaryev@gmail.com>

dianpopa dismissed stale reviews from ioanachirca and lauralt via e11bd9b May 29, 2020 14:41

lauralt approved these changes May 29, 2020

View reviewed changes

ioanachirca approved these changes May 29, 2020

View reviewed changes

ioanachirca merged commit b892fbe into firecracker-microvm:master May 29, 2020

edigaryev deleted the improve-cmdline-arguments branch May 29, 2020 15:03

lauralt mentioned this pull request Jun 2, 2020

jailer: increase code coverage #1940

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve command-line arguments #1862

Improve command-line arguments #1862

edigaryev commented May 4, 2020

edigaryev commented May 4, 2020

shioyama18 commented May 5, 2020

ioanachirca left a comment

edigaryev commented May 27, 2020

lauralt May 29, 2020

edigaryev May 29, 2020

lauralt May 29, 2020

edigaryev May 29, 2020

lauralt commented May 29, 2020 •

edited

Loading

edigaryev commented May 29, 2020

lauralt left a comment

lauralt commented May 29, 2020

edigaryev commented May 29, 2020

lauralt commented May 29, 2020

lauralt commented May 29, 2020

Improve command-line arguments #1862

Improve command-line arguments #1862

Conversation

edigaryev commented May 4, 2020

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

edigaryev commented May 4, 2020

shioyama18 commented May 5, 2020

ioanachirca left a comment

Choose a reason for hiding this comment

edigaryev commented May 27, 2020

lauralt May 29, 2020

Choose a reason for hiding this comment

edigaryev May 29, 2020

Choose a reason for hiding this comment

lauralt May 29, 2020

Choose a reason for hiding this comment

edigaryev May 29, 2020

Choose a reason for hiding this comment

lauralt commented May 29, 2020 • edited Loading

edigaryev commented May 29, 2020

lauralt left a comment

Choose a reason for hiding this comment

lauralt commented May 29, 2020

edigaryev commented May 29, 2020

lauralt commented May 29, 2020

lauralt commented May 29, 2020

lauralt commented May 29, 2020 •

edited

Loading