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

Update README to use a Docker image matching the one used in CI. #95

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

niting
Copy link
Contributor

@niting niting commented May 18, 2020

The local test reproduction on docker, as specified in the README, fails currently with the following error:

error[E0133]: access to union field is unsafe and requires unsafe function or block
--> src/ioctls/vm.rs:1331:9
|
1331 | irqchip.chip.pic.irq_base = 10;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ access to union field
|
= note: the field may not be properly initialized: using uninitialized data will cause undefined behavior

This PR fixes the local test failure by appropriately marking the code as unsafe.

@andreeaflorescu
Copy link
Member

Hey @niting. For some reason in the CI that unsafe doesn't yield an error. Maybe it is because of the rust version, or something else. I can look into it next week. Sorry for the late reply.

@niting
Copy link
Contributor Author

niting commented May 24, 2020

Thank you for looking into it! This indeed was an older toolchain issue - the particular error was fixed in rust-lang/rust#42083 and I confirmed that this doesn't happen anymore with the newer version of the Docker image. I have accordingly updated the commit. I am not sure why coverage runs still fail on the commit.

@niting niting changed the title Fix local test reproduction broken by unsafe code. Update README to use a Docker image matching the one used in CI. May 24, 2020
@andreeaflorescu
Copy link
Member

Coverage on arm is pretty much random at this point. I'll update the CI to skip that test until we find out what's happening there. You can also manually update to the value it expects.

To merge this PR, can you please sign your commit?
git commit --amend -s --no-edit

Thanks.

Signed-off-by: niting <niting@google.com>
@niting
Copy link
Contributor Author

niting commented May 27, 2020

Done, thanks! I went ahead and signed the commit but skipped the manual update.

@niting
Copy link
Contributor Author

niting commented May 30, 2020

@andreeaflorescu and @aghecenco thank you for the reviews. Do I need to update the test coverage for the PR to be merged? Or does it require an additional review?

@andreeaflorescu
Copy link
Member

@niting I'll open a PR to drop that coverage-arm test. We'll merge that one first, then update the branch for this one, then merge it.

I just didn't got around to open that PR just yet :))

We'll be doing that soon. No action required from your part. Thanks!

@andreeaflorescu andreeaflorescu merged commit 1072239 into rust-vmm:master Jun 2, 2020
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