Import crosvm FDT writer #1

danielverkamp · 2021-03-29T19:22:38Z

Signed-off-by: Daniel Verkamp dverkamp@chromium.org

andreeaflorescu · 2021-03-30T07:15:57Z

@danielverkamp thanks for importing the code! I just enabled the CI on this repository, it seemed like I missed doing that before. We will need to also update the rust-vmm-ci as it s now using a really ancient version which means that we do not have all the tests running, and we're using an old Rust version. I'll prepare a separate PR for that as there are other tiny fixes required for the CI to pass (for example, we need a coverage file to be committed to the repository).

andreeaflorescu · 2021-03-30T07:20:23Z

src/writer.rs

+        assert_eq!(
+            fdt.finish(0x68).unwrap(),
+            [
+                0xd0, 0x0d, 0xfe, 0xed, // 0000: magic (0xd00dfeed)


I briefly played with the FdtWriter you wrote in a POC for vmm-reference. So far things seem to work quite nicely.

The only problem I spotted so far is that comparing large arrays (len > 32) does not seem to be possible in Rust stable. What Rust version are you using for building crosvm? The CI will fail in rust-vmm-ci as well as we are using Rust 1.46 for running all tests.

I did a rather quick & dirty fix such that the build passes in vmm-reference. Here is my commit: andreeaflorescu/vmm-reference@6563f0d

We are currently using Rust 1.47.0 in the Chrome OS build environment; this looks like the relevant change in the standard library: rust-lang/rust#74060

If we want to maintain compatibility with older Rust versions, the workaround in your commit looks fine to me - thanks!

For now we are constrained to 1.46 Rust because the aach64-musl target does not work on any newer versions. Here is the issue for reference: rust-lang/rust#79791

It indeed works with newer versions of Rust, sorry for the confusion.

If the workaround sounds good to you, we can just temporarily use it, track the aforementioned issue and just revert it when we can update the Rust version.

Keeping support for old versions sounds good - how do you prefer to merge the change in that case? Should I add your commit to this PR, or merge this one and then yours as a separate PR? (not sure how the normal GitHub and CI flow works)

We will need to update this PR in order to be able to merge it.

A few things are needed to make the CI run, and the tests pass:

The first thing we need to do is to update the rust-vmm-ci as described here: https://github.com/rust-vmm/community/blob/master/CONTRIBUTING.md#updating-the-rust-vmm-ci

Run the coverage test and update the coverage config file (we are lagging a bit behind with the documentation on this one; I can help with this)

Make the CI green (there might be tests that are failing - for example style, clippy; we need to fix those to be able to merge the PR)

You'll need 2 reviewers that approve this PR (I'll be one of them, and we can find another one during today's sync meeting)

I can help you with the commits that are related to the CI. Would you mind if I push into your fork the fix? Otherwise, I can also just create a branch from the commits in your PR, and add the fixes on top. Let me know what works best for you. Thanks!

Pushing into my fork sounds good - I have sent you a GitHub invitation to allow (I think) direct access.

Thanks @danielverkamp. I've pushed a few commits that should fix the CI. I did not find another reviewer for this PR because, well, it was a very lonely rust-vmm sync meeting since for everybody else there were the Easter holidays 🤣.

Can you take a look and let me know if it looks good to you? I'll also take a more in depth look at your initial commit, and try to find some more reviewers. Thanks!

The additional changes look fine to me - thank you!

(The array comparison helper using floating point, which I didn't notice before, looks a bit unusual, but it should not cause any problems for reasonably-sized arrays that we will write by hand.)

It is indeed weird. I was a bit lazy. I just adjusted it to use a dummy ceil implementation that works directly on usize, and updated that commit with a force push.

andreeaflorescu

Just a few high level comments, I still need to look at the implementation details. I'll continue my review at the end of the week, sorry for the delay.

src/lib.rs

andreeaflorescu · 2021-04-07T06:58:43Z

src/writer.rs

+/// ```rust
+/// use vm_fdt::FdtWriter;
+///
+/// # fn main() -> vm_fdt::FdtWriterResult<()> {


We are trying to move away from using functions in examples because they can silently fail, leaving examples not working. Would that make sense? I am trying to understand what would be the benefit of having the example under the main function besides being able to use ? and not unwrap.

The only reason I put it into a function was for ? as you mentioned - I'm not sure using unwrap in an example is ideal, since users will likely copy and paste it, but I can switch to unwrap if that's the preferred style.

I think that makes sense. So to get the best of both worlds, what do you say about the following:

rename the function to something like fdt_example

call the function and unwrap (this should be as a comment such that the users don't see it in the documentation; this way if we did something weird in the example, the doc tests will fail.

/// # fn fdt_example() -> vm_fdt::FdtWriterResult<()> { /// let mut fdt = FdtWriter::new(&[]); //// ..... /// let dtb = fdt.finish(0x1000)?; /// # Ok(()) /// # } /// # let _ = fdt_example().unwrap();

Sorry, I totally missed this followup comment - I updated the docs to match.

src/writer.rs

danielverkamp

Thanks for the review! I have force-pushed my changes on the branch in my fork (hopefully that is the convention - if not, I am happy to use another method, e.g. pushing new commits on top of the original change).

src/lib.rs

src/writer.rs

danielverkamp · 2021-04-08T22:37:26Z

src/writer.rs

+/// ```rust
+/// use vm_fdt::FdtWriter;
+///
+/// # fn main() -> vm_fdt::FdtWriterResult<()> {


The only reason I put it into a function was for ? as you mentioned - I'm not sure using unwrap in an example is ideal, since users will likely copy and paste it, but I can switch to unwrap if that's the preferred style.

andreeaflorescu · 2021-04-09T06:40:31Z

Thanks for the review! I have force-pushed my changes on the branch in my fork (hopefully that is the convention - if not, I am happy to use another method, e.g. pushing new commits on top of the original change).

We don't really have a convention. If you'd like to stack commits, and just squashing them before we merge the PR that also works out.

danielverkamp · 2021-04-09T20:25:56Z

It looks like the coverage changed because of the new fmt implementation; I uploaded a new version with updated coverage information (also added a few more trivial tests to cover some missing cases).

andreeaflorescu · 2021-04-12T06:46:53Z

It looks like the coverage changed because of the new fmt implementation; I uploaded a new version with updated coverage information (also added a few more trivial tests to cover some missing cases).

You can ignore the coverage test for now while the PR is still in review, and we can just update it when we're ready to merge it.

jiangliu · 2021-04-13T02:41:34Z

CODEOWNERS

@@ -1,2 +1,3 @@
 # Add the list of code owners here (using their GitHub username)
 * gatekeeper-PullAssigner
+* danielverkamp


seems it should be "@danielverkamp".

Updated, thanks.

jiangliu · 2021-04-13T02:53:41Z

Cargo.toml

 version = "0.1.0"
-authors = [TODO]
+authors = ["The Chromium OS Authors"]
 license = "Apache-2.0 OR BSD-3-Clause"


All files are licensed under BSD-3-Clause, should we remove "Apache-2.0"? Or change source to " Apache-2.0 OR BSD-3-Clause"?
I remember there was a discussion about preferring "Apache-2.0 OR BSD-3-Clause".

I can license them under "Apache-2.0 OR BSD-3-Clause" if that is preferred - updated to use the SPDX file header (please sanity check that this looks correct).

slp · 2021-04-15T13:31:12Z

This looks good to me. I'm eager to use this libkrun. ;-)

Once the comments raised by @jiangliu are addressed and coverage level is fixed, I'm OK with merging this PR.

slp · 2021-04-21T05:33:19Z

@andreeaflorescu CI is failing but I can't see the output (I get a 404). Do you know what could be happening?

andreeaflorescu · 2021-04-21T06:39:42Z

@andreeaflorescu CI is failing but I can't see the output (I get a 404). Do you know what could be happening?

Oops, sorry about that! I forgot to make the pipeline public. It should work now.

slp · 2021-04-21T07:50:36Z

@andreeaflorescu I can see it now, thanks!

@danielverkamp CI complains about a couple of minor format errors, and now that the PR is almost final, I'd say it's time to adjust the expected coverage value. Thanks!

danielverkamp · 2021-04-21T20:08:10Z

I've updated the coverage and gave it a rustfmt pass - thanks for the reviews!

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>

24d66cd update clippy check ebc7016 exclude dependabot from the 50/72 commit rule 63fdf0f update rust-vmm-container 98a26fe Typo fix in README.md 631c82a mount tmp in test pipelines 03fcf08 don't run commit test for repos specified with git 3b7377c fixed typo in readme c9430ee add gitignore file e1108f1 buildkite: re-enable cargo audit test 02004b5 Add a flag that saves the coverage output dir 97025bd buildkite: Skip lines should be shorter than 70 chars c83003c buildkite: Skip cargo audit check temporarily c8cf2b7 buildkite: Fix audit label indentation b3acb30 Fixes: rust-vmm/rust-vmm-ci#8 bedc32b Add --workspace flag to cargo check too 3ea5f2b improve a bit error messages for commit test cd90a63 Add support for workspace tests 9dd386c readme update: cosmetic changes 265df53 Coverage test: keep stdin open 2d3bb05 add myself to codeowners e58ea74 Fix kcov_ouput_dir typo in test_coverage.py d62d781 fix buildkite typos in readme 0fc8ced refactor test_benchmark.py 741b894 checkout to PR branch before finishing test_bench 645a5c3 test_bench: don't crash when no bench on master bd32544 Fetch origin in benchmark test 35beb91 Fix commit message test 53427aa benchmarks: add test that can run at every PR abd2c90 Add test for commit message format fe859f4 Update container image to v6 75d7254 run cargo check on all features 7e3f307 skip coverage-arm test cd7096e Enable rust-vmm coverage test in CI c309d06 buildkite: Move to the rustvmm/dev v4 container c85a8da buildkite: Remove clippy test on aarch64 Signed-off-by: Andreea Florescu <fandree@amazon.com>

Comparing arrays with len > 32 is not allowed in rust rust 1.46. Added a workaround for testing these arrays by splitting them in groups of maximum 32 elems. Signed-off-by: Andreea Florescu <fandree@amazon.com>

The file is needed by the rust-vmm-ci coverage integration test. Signed-off-by: Andreea Florescu <fandree@amazon.com>

The linker was unable to find __addtf3, __multf3 and __subtf3. Added target-feature=+crt-static and link-arg=-lgcc as a temporary workaround. This seems to be the accepted fix in the Rust community: rust-lang/compiler-builtins#201 A permanent fix is yet to be implemented in the Rust compiler. Signed-off-by: Andreea Florescu <fandree@amazon.com>

slp

LGTM, thanks!

andreeaflorescu reviewed Mar 30, 2021

View reviewed changes

andreeaflorescu mentioned this pull request Apr 6, 2021

Bump rust-vmm-ci from bb1cd14 to 24d66cd #2

Closed

andreeaflorescu force-pushed the initial-import branch from 879ad2b to 2f0f6b6 Compare April 7, 2021 06:50

andreeaflorescu reviewed Apr 8, 2021

View reviewed changes

danielverkamp force-pushed the initial-import branch from 2f0f6b6 to cc4c3c0 Compare April 8, 2021 22:38

danielverkamp commented Apr 8, 2021

View reviewed changes

danielverkamp force-pushed the initial-import branch from cc4c3c0 to 11089f0 Compare April 8, 2021 22:40

danielverkamp force-pushed the initial-import branch from 11089f0 to fc7d751 Compare April 9, 2021 20:24

danielverkamp force-pushed the initial-import branch from fc7d751 to 3c0d109 Compare April 9, 2021 20:29

andreeaflorescu requested review from jiangliu and slp April 12, 2021 08:32

jiangliu reviewed Apr 13, 2021

View reviewed changes

danielverkamp force-pushed the initial-import branch 2 times, most recently from b0dfa57 to b366164 Compare April 16, 2021 22:58

danielverkamp force-pushed the initial-import branch from b366164 to 74af64a Compare April 21, 2021 20:06

danielverkamp and others added 5 commits April 21, 2021 13:42

Import crosvm FDT writer

59c25aa

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>

fix fdt tests that were comparing large arrays

f1a7c37

Comparing arrays with len > 32 is not allowed in rust rust 1.46. Added a workaround for testing these arrays by splitting them in groups of maximum 32 elems. Signed-off-by: Andreea Florescu <fandree@amazon.com>

add coverage file

99de710

The file is needed by the rust-vmm-ci coverage integration test. Signed-off-by: Andreea Florescu <fandree@amazon.com>

danielverkamp force-pushed the initial-import branch from 74af64a to 51fd8c7 Compare April 21, 2021 20:42

slp approved these changes Apr 22, 2021

View reviewed changes

jiangliu approved these changes Apr 23, 2021

View reviewed changes

jiangliu approved these changes Apr 27, 2021

View reviewed changes

andreeaflorescu approved these changes Apr 28, 2021

View reviewed changes

andreeaflorescu merged commit 8d81555 into rust-vmm:master Apr 28, 2021

danielverkamp deleted the initial-import branch September 2, 2021 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import crosvm FDT writer #1

Import crosvm FDT writer #1

danielverkamp commented Mar 29, 2021

andreeaflorescu commented Mar 30, 2021

andreeaflorescu Mar 30, 2021 •

edited

Loading

danielverkamp Mar 30, 2021

andreeaflorescu Mar 31, 2021

danielverkamp Apr 2, 2021

andreeaflorescu Apr 5, 2021

danielverkamp Apr 5, 2021

andreeaflorescu Apr 6, 2021

danielverkamp Apr 6, 2021

andreeaflorescu Apr 7, 2021

andreeaflorescu left a comment

andreeaflorescu Apr 7, 2021

danielverkamp Apr 8, 2021

andreeaflorescu Apr 9, 2021

danielverkamp Apr 21, 2021

danielverkamp left a comment

danielverkamp Apr 8, 2021

andreeaflorescu commented Apr 9, 2021

danielverkamp commented Apr 9, 2021

andreeaflorescu commented Apr 12, 2021

jiangliu Apr 13, 2021

danielverkamp Apr 16, 2021

jiangliu Apr 13, 2021

danielverkamp Apr 16, 2021

slp commented Apr 15, 2021

slp commented Apr 21, 2021

andreeaflorescu commented Apr 21, 2021 •

edited

Loading

slp commented Apr 21, 2021

danielverkamp commented Apr 21, 2021

slp left a comment

Import crosvm FDT writer #1

Import crosvm FDT writer #1

Conversation

danielverkamp commented Mar 29, 2021

andreeaflorescu commented Mar 30, 2021

andreeaflorescu Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielverkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu commented Apr 9, 2021

danielverkamp commented Apr 9, 2021

andreeaflorescu commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slp commented Apr 15, 2021

slp commented Apr 21, 2021

andreeaflorescu commented Apr 21, 2021 • edited Loading

slp commented Apr 21, 2021

danielverkamp commented Apr 21, 2021

slp left a comment

Choose a reason for hiding this comment

andreeaflorescu Mar 30, 2021 •

edited

Loading

andreeaflorescu commented Apr 21, 2021 •

edited

Loading