vmm-sys-util #1

liujing2 · 2019-03-20T08:44:41Z

This includes the utilities that directly use libc from crosvm and firecracker.

andreeaflorescu

Pass 1, will do another pass soonish.

andreeaflorescu · 2019-03-20T09:51:37Z

src/errno.rs

+// Copyright 2019 Intel Corporation. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+// TODO: This module is not used now. Avoid the warning.


I believe it shouldn't throw a warning if you are not using this module in the vmm-sys-util crate. All modules from this crate should be re-exported with pub mod in src/lib.rs.

Thank you for the quick reviewing! I'll fix the comments in codes.

andreeaflorescu · 2019-03-20T09:53:16Z

src/lib.rs

+#[macro_use]
+pub mod ioctl;
+
+mod errno;


I think the modules here should be re-exported with pub. These modules are not for internal use in the vmm-sys-util crate but they're going to be used by other crates.

andreeaflorescu · 2019-03-20T10:02:53Z

src/errno.rs

@@ -0,0 +1,86 @@
+// Copyright 2019 Intel Corporation. All Rights Reserved.


Any reason why we need this module instead of using std::io::Error?
In Firecracker we replaced it with std::io::Error because it wasn't producing human readable errors.

For example, let's say you do not have enough permissions to open /dev/kvm.

the error message with errno.rs: Cannot open /dev/kvm. errno: 13

the error message with std::io::Error: Cannot open /dev/kvm. Error: permission denied.

The nice part about this error type is that it is fully compatible with os error code thrown off by system calls. If one were to put an os error code into an io::Error, it might be hard to get the os error back out in upper parts of the call stack. Using io::Error::raw_os_error returns an Option that either requires unwrap or an extra match just to extract the code. Using io::Error::kind is tricky because it has variants which are not possible to get from a syscall (UnexpectedEof), or are mismatched by the std implementation that converts error numbers to ErrorKind variants. Additionally, some error codes have no correspondence with an ErrorKind variant other than Other.

If human error messaging is all that is needed to make this module acceptable, I suggest using what we have in crosvm today for Error:

impl Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { io::Error::from_raw_os_error(self.0).fmt(f) } }

andreeaflorescu · 2019-03-20T10:07:23Z

README.md

+# vmm-sys-util
+This crate is a collection of modules that essentially provide helpers and utilities.


nit: essentially is just a filler word, can you remove it?

Yes. Will do it.

andreeaflorescu · 2019-03-20T10:09:31Z

src/eventfd.rs

+
+//TODO: Firecracker uses io::Error instead of sys_util::Error for Result<T, E>. Need discussion here.
+
+#![allow(unused)]


This should not be necessary once we export the module as public in src/lib.rs.

Right! Will remove it

andreeaflorescu · 2019-03-20T10:53:28Z

src/eventfd.rs

+/// A safe wrapper around a Linux eventfd (man 2 eventfd).
+///
+/// An eventfd is useful because it is sendable across processes and can be used for signaling in
+/// and out of the KVM API. They can also be polled like any other file descriptor.


nit-ish: I would remove the reference to the KVM API here. It is not super relevant in the context of rust-vmm where we will also have other hypervisors. I believe the comment is a bit outdated.

andreeaflorescu · 2019-03-20T11:01:46Z

src/eventfd.rs

+}
+
+impl EventFd {
+    /// Creates a new blocking EventFd with an initial value of 0.


I don't think this comment is accurate. We are creating the eventfd with EFD_NONBLOCK.
I noticed that in CrosVM they are opening it without the flag so maybe it would make sense to pass the additional flags as parameters.

Yep. Let me change the flag to be a parameter for new() and test the new(EFD_NONBLOCK) in #[test]. Do you think we need double test the new(0) for each #[test]?

Hmm, I'll look at the code. It is always nice to test all the usecases, now it depends whether it adds any value.

andreeaflorescu · 2019-03-20T16:56:27Z

@liujing2 please also add the Travis configuration file. Here is the one from the vm-memory crate: https://github.com/rust-vmm/vm-memory/blob/master/.travis.yml

andreeaflorescu · 2019-03-29T08:55:50Z

Cargo.toml

@@ -0,0 +1,9 @@
+[package]
+name = "libc-utils"


Name should probably be vmm-sys-utils

andreeaflorescu · 2019-03-29T09:43:09Z

src/timerfd.rs

+/// A safe wrapper around a Linux timerfd (man 2 timerfd_create).
+pub struct TimerFd(File);
+
+impl TimerFd {


I haven't check the other docs, but I think it is a good idea to have the documentation split in summary and description.

For new() it would look something like this:

/// Create a new `TimerFd`. /// /// The timer is initially disarmed and must be armed by calling `reset()`.

Then we could also add links to the structures and methods, but I guess it is optional because it does make the code comments harder to read with the advantage of making the html docs prettier:

/// Create a new [`TimerFd`](struct.TimerFd.html). /// /// The timer is initially disarmed and must be armed by calling [`reset`](struct.TimerFd.html#method.reset).

P.S. I haven't checked that the documentation links are correct.

Let me split the summary and description. The link might be optional and I found not all of the linux man is appended in original codes.

Hmm, I don't think I understand, I meant inside links to structures and functions that you are referring to in the comments.
https://rust-lang-nursery.github.io/api-guidelines/documentation.html#prose-contains-hyperlinks-to-relevant-things-c-link

@andreeaflorescu Thanks for the guide. I misunderstood. I'll update a new version today.

andreeaflorescu · 2019-03-29T09:44:51Z

src/timerfd.rs

+}
+
+#[cfg(test)]
+mod tests {


Please prefix tests with test_ (test_one_shot). This is a good way to visually separate tests from helper methods.

OK, I will add the prefix for every tests in these files.

sboeuf

Could you please be more verbose in each commit message? We should be able to quickly understand what was introduced by each commit only looking at the commit messages IMO.

jiangliu · 2019-04-13T05:12:32Z

Cargo.lock

+]
+
+[metadata]
+"checksum libc 0.2.49 (registry+https://github.com/rust-lang/crates.io-index)" = "413f3dfc802c5dc91dc570b05125b6cda9855edfaa9825c9849807876376e70e"


There was a discussion about whether Cargo.lock should be committed. I remember the conclusion is not to commit for library crate and only commit for binary crate.

Thanks for the reviewing. Will ignore it in next update.

jiangliu · 2019-04-13T05:13:04Z

Cargo.toml

+name = "libc-utils"
+version = "0.1.0"
+authors = ["Jing Liu <jing2.liu@linux.intel.com>"]
+edition = "2018"


Seems a little risky to force 2018 edition.

Got it. Remove it and add license BTW.

jiangliu · 2019-04-13T05:14:43Z

src/errno.rs

@@ -0,0 +1,86 @@
+// Copyright 2019 Intel Corporation. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0


Now it's suggested to dual license for Apache-2.0 and MIT. Please help to add support of MIT.

Now it's suggested to dual license for Apache-2.0 and MIT. Please help to add support of MIT.

@jiangliu I just updated the codes regarding to the license and other review comments.
BTW, for the vhost utility, could you explain your request in details so I can help?

jiangliu · 2019-04-13T05:15:29Z

src/errno.rs

+/// Returns the last `errno` as a [`Result`] that is always an error.
+///
+/// [`Result`]: type.Result.html
+///


Should we remove the blank lines?

I think blank line is no need here. The same as others.

liujing2 · 2019-04-16T05:14:41Z

Could you please be more verbose in each commit message? We should be able to quickly understand what was introduced by each commit only looking at the commit messages IMO.

Sure. Let me append more.

jiangliu · 2019-04-16T09:49:42Z

Currently the vhost crate makes use of the EventFd and ioctl macros from the sys_util crate, seems they are available in the vmm-sys-util already.

…

On Apr 16, 2019, at 5:40 PM, liujing2 ***@***.*** ***@***.***>> wrote: @liujing2 commented on this pull request. In src/errno.rs <#1 (comment)>: > @@ -0,0 +1,86 @@ +// Copyright 2019 Intel Corporation. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 Now it's suggested to dual license for Apache-2.0 and MIT. Please help to add support of MIT. @jiangliu <https://github.com/jiangliu> I just updated the codes regarding to the license and other review comments. BTW, for the vhost utility, could you explain your request in details so I can help? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB14_A3AoZQL225N9B1pkL2iTCEtjfwOks5vhZp-gaJpZM4b-wo4>.

liujing2 · 2019-04-17T03:12:08Z

Codes updated with license MIT and review comments. Thanks!

jiangliu · 2019-04-17T03:21:23Z

.travis.yml

+script:
+  - cargo build -- release
+  - cargo test
+  - cargo fmt --all -- --check


Do we need to check clippy result here ?

cargo clippy --all-targets --all-features -- -D warnings

Yep, I just updated PR with a separate commit on top, about solving the clippy issue. Thanks.

jiangliu · 2019-04-17T03:30:03Z

src/errno.rs

+//
+// Portions Copyright 2017 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the THIRD-PARTY file.


Seems the BSD license file is missing from the crate.

BSD license file is added.

sboeuf

LGTM, thank you @liujing2!

sboeuf · 2019-04-23T23:22:00Z

@zachreizner @andreeaflorescu @rbradford PTAL

sameo

LGTM

rbradford · 2019-04-30T09:42:13Z

LICENSE.MIT

@@ -0,0 +1,21 @@
+MIT License


What code is coming under MIT vs BSD or Apache 2.0?

It seems currently dual license for Apache-2.0 and MIT are supported for rust-vmm. See the comments BSD is copied from crosvm.

@liujing2 I think it would be wiser to remove the MIT license for now and only keep the Apache 2.0 and the 3 clause BSD one.

Will remove MIT license.

sameo · 2019-05-06T23:33:49Z

@liujing2 I sent you a PR for adding a few more utilities to that crate, mostly for supporting disk image format crates like e.g. QCOW.
See liujing2#1

sameo · 2019-05-06T23:28:06Z

LICENSE.BSD

@@ -0,0 +1,27 @@
+// Copyright 2017 The Chromium OS Authors. All rights reserved.


Could we please rename that file LICENSE-BSD-3-Clause and the Apache one LICENSE-APACHE? Just to be consistent with the already existing crates?

Will do that.

sameo · 2019-05-06T23:30:05Z

LICENSE.MIT

@@ -0,0 +1,21 @@
+MIT License


@liujing2 I think it would be wiser to remove the MIT license for now and only keep the Apache 2.0 and the 3 clause BSD one.

Add and correct basic support files for the vmm-sys-util crate including: - files generated by Cargo - license files - gitignore Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>

The errno module wraps an interface to the error codes. The eventfd module is a wraper around the Linux eventfd. Use result::Result<EventFd, io::Error> instead of result::Result<EventFd, errno::Errno> as return type. The terminal module is for configuring pseudo-terminals in raw mode. The signal module is for registering signal handlers and for signalling threads. The ioctl module is factoring out common ioctl wrappers and macros. This is derived from crosvm code base e4ece32798ba436fbfa88466c571f46dab99e2be and firecracker code base 1fdde1997fc763601f8a88399bc426bd5d4097cd. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> --- Change Log Add license. Append prefix test_ for each test. Add verbose commit messages.

To use different init flag(e.g. SA_RESTART, SA_SIGINFO) for sigaction, add a parameter to register_signal_handler and a function to set this flag. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>

Wrapping the libc timerfd for the usage of setting timer, waiting expiring, disarm timer. This is derived from crosvm code base e4ece32798ba436fbfa88466c571f46dab99e2be. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> --- Change Log: Add license. Split the summary and description for method. Add link for structures and functions. Append prefix test_ for tests.

The poll module addis the Poller object for waiting on mutliple file descriptors at once. Add macro handle_eintr_errno! to check the epoll_wait return value. The syslog module is for logging, temporarily defining the SYS_getpid syscall value. This is derived from crosvm code base e4ece32798ba436fbfa88466c571f46dab99e2be. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com> --- Change Log Append prefix test_ for tests.

This is needed for disk image format support like e.g. QCOW2. Copied verbatim from crosvm 107edb3e. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

Several places are improved to suppress the clippy warnings. The changes of type casting in timerfd module are based on crosvm latest code base b1de6323ab8c96c52a60e0ff735e4bbb8a8464c9. The write_zeroes and seek_hole clippy warnings are only on test code, we can safely allow them. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

jiangliu · 2019-05-07T09:47:24Z

Seems file_traits, wrtite_zeros, seek_hole and fallocate are related to each other, how about combine them into one file?

jiangliu · 2019-05-19T16:28:36Z

It fails to compile with "cargo build --target x86_64-unknown-linux-musl" as below. GNU library is ok.
error[E0432]: unresolved imports libc::SEEK_DATA, libc::SEEK_HOLE
--> /usr/local/rust/git/checkouts/vmm-sys-util-5509361781321914/766db44/src/seek_hole.rs:11:28
|
11 | use libc::{lseek64, ENXIO, SEEK_DATA, SEEK_HOLE};
| ^^^^^^^^^ ^^^^^^^^^ no SEEK_HOLE in the root
| |
| no SEEK_DATA in the root. Did you mean to use SEEK_SET?

error: aborting due to previous error

For more information about this error, try rustc --explain E0432.

liujing2 · 2019-05-20T06:27:34Z

It fails to compile with "cargo build --target x86_64-unknown-linux-musl" as below. GNU library is ok.
error[E0432]: unresolved imports libc::SEEK_DATA, libc::SEEK_HOLE
--> /usr/local/rust/git/checkouts/vmm-sys-util-5509361781321914/766db44/src/seek_hole.rs:11:28
|
11 | use libc::{lseek64, ENXIO, SEEK_DATA, SEEK_HOLE};
| ^^^^^^^^^ ^^^^^^^^^ no SEEK_HOLE in the root
| |
| no SEEK_DATA in the root. Did you mean to use SEEK_SET?

@jiangliu According to the MUSL libc which is used by rust musl build, we can find that MUSL libc doesn't have SEEK_HOLE and SEEK_DATA support.

musl does not define the SEEK_HOLE and SEEK_DATA constants. We use the libc one when using glibc and we define them when building for musl. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sboeuf · 2019-05-22T22:58:11Z

@zachreizner @dgreid PTAL

jiangliu · 2019-05-27T12:10:49Z

Could we also reexport libc::{EFD_SEMAPHORE, EFD_SEMAPHORE, EFD_CLOEXEC} in the eventfd.rs, so it get a little easier for use.

sameo · 2019-05-28T09:25:13Z

Testing a potential fix...

Using the File wrappers leads to poisoned pointers when running under a tmpfs based /dev mount. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

The vmm-sys-util test make heavy use of temporary dirs and files. Running that under a container overlay leads to erratic behaviours and this is fixed by using a real tmpfs mount into the container's /tmp. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sameo · 2019-05-28T09:30:55Z

Testing a potential fix...

Seems to be working.

sameo · 2019-05-28T09:32:23Z

Could we also reexport libc::{EFD_SEMAPHORE, EFD_SEMAPHORE, EFD_CLOEXEC} in the eventfd.rs, so it get a little easier for use.

@jiangliu Do you mind if we merge that one first and then send that as a follow up PR?

jiangliu · 2019-05-28T09:46:19Z

Could we also reexport libc::{EFD_SEMAPHORE, EFD_SEMAPHORE, EFD_CLOEXEC} in the eventfd.rs, so it get a little easier for use.

@jiangliu Do you mind if we merge that one first and then send that as a follow up PR?

sure, actually I have three patches upon this patch set:)

sameo · 2019-05-28T09:58:54Z

Could we also reexport libc::{EFD_SEMAPHORE, EFD_SEMAPHORE, EFD_CLOEXEC} in the eventfd.rs, so it get a little easier for use.

@jiangliu Do you mind if we merge that one first and then send that as a follow up PR?

sure, actually I have three patches upon this patch set:)

Excellent :) Could you please give this PR another review? Adding new commits dismissed the previous reviews...

sboeuf · 2019-05-28T16:01:56Z

Looks like this PR is ready to be merged :)

jiangliu · 2019-05-28T16:23:03Z

Looks like this PR is ready to be merged :)

Great, the vhost crate could move on once this has been merged:)

andreeaflorescu reviewed Mar 20, 2019

View reviewed changes

andreeaflorescu reviewed Mar 29, 2019

View reviewed changes

sboeuf reviewed Apr 12, 2019

View reviewed changes

jiangliu reviewed Apr 13, 2019

View reviewed changes

liujing2 force-pushed the master branch from bf97be5 to 18952f1 Compare April 16, 2019 09:36

liujing2 force-pushed the master branch 3 times, most recently from a651f7a to 8663f2e Compare April 17, 2019 03:10

jiangliu reviewed Apr 17, 2019

View reviewed changes

liujing2 force-pushed the master branch from 8663f2e to 7d898ac Compare April 19, 2019 07:10

jiangliu previously approved these changes Apr 23, 2019

View reviewed changes

sboeuf previously approved these changes Apr 23, 2019

View reviewed changes

serban300 mentioned this pull request Apr 24, 2019

implement KvmVec struct rust-vmm/kvm#6

Closed

sameo previously approved these changes May 1, 2019

View reviewed changes

rbradford requested changes May 1, 2019

View reviewed changes

sameo reviewed May 6, 2019

View reviewed changes

Jing Liu added 5 commits May 7, 2019 09:02

vmm-sys-util: Add basic files

e570ddb

Add and correct basic support files for the vmm-sys-util crate including: - files generated by Cargo - license files - gitignore Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>

vmm-sys-util: Add flag parameter for signal module

d6b8535

To use different init flag(e.g. SA_RESTART, SA_SIGINFO) for sigaction, add a parameter to register_signal_handler and a function to set this flag. Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>

liujing2 dismissed stale reviews from sameo, sboeuf, and jiangliu via 6ee1973 May 7, 2019 01:17

Samuel Ortiz and others added 3 commits May 7, 2019 07:40

seek_hole: Add a trait for seeking holes in a file

08daac2

This is needed for disk image format support like e.g. QCOW2. Copied verbatim from crosvm 107edb3e. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

vmm-sys-util: Add buildkite based CI

766db44

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

liujing2 force-pushed the master branch from 90f39f9 to 766db44 Compare May 7, 2019 09:27

andreeaflorescu mentioned this pull request May 20, 2019

virtio core support rust-vmm/vm-virtio#1

Merged

seek_hole: Fix musl compilation

45d47cf

musl does not define the SEEK_HOLE and SEEK_DATA constants. We use the libc one when using glibc and we define them when building for musl. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

jiangliu previously approved these changes May 22, 2019

View reviewed changes

sboeuf previously approved these changes May 22, 2019

View reviewed changes

sameo dismissed stale reviews from sboeuf and jiangliu via b3dd077 May 28, 2019 09:24

Samuel Ortiz added 2 commits May 28, 2019 11:26

syslog: Use libc directly to get lowest fd

d660a92

Using the File wrappers leads to poisoned pointers when running under a tmpfs based /dev mount. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

buildkite: Use a real tmpfs

700a05b

The vmm-sys-util test make heavy use of temporary dirs and files. Running that under a container overlay leads to erratic behaviours and this is fixed by using a real tmpfs mount into the container's /tmp. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sameo previously approved these changes May 28, 2019

View reviewed changes

sameo dismissed their stale review via 700a05b May 28, 2019 09:31

sameo force-pushed the master branch from b3dd077 to 700a05b Compare May 28, 2019 09:31

rbradford approved these changes May 28, 2019

View reviewed changes

jiangliu approved these changes May 28, 2019

View reviewed changes

andreeaflorescu merged commit 60fe35b into rust-vmm:master May 28, 2019

		@@ -0,0 +1,86 @@
		// Copyright 2019 Intel Corporation. All Rights Reserved.

		# vmm-sys-util
		This crate is a collection of modules that essentially provide helpers and utilities.


		//TODO: Firecracker uses io::Error instead of sys_util::Error for Result<T, E>. Need discussion here.

		#![allow(unused)]

		@@ -0,0 +1,86 @@
		// Copyright 2019 Intel Corporation. All Rights Reserved.
		// SPDX-License-Identifier: Apache-2.0

		@@ -0,0 +1,27 @@
		// Copyright 2017 The Chromium OS Authors. All rights reserved.

vmm-sys-util #1

vmm-sys-util #1

Conversation

liujing2 commented Mar 20, 2019

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

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 Mar 20, 2019 • 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

andreeaflorescu commented Mar 20, 2019

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

liujing2 Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liujing2 Apr 16, 2019 • 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

liujing2 commented Apr 16, 2019

jiangliu commented Apr 16, 2019 via email

liujing2 commented Apr 17, 2019

Choose a reason for hiding this comment

liujing2 Apr 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf left a comment

Choose a reason for hiding this comment

sboeuf commented Apr 23, 2019

sameo 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

sameo commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented May 7, 2019

jiangliu commented May 19, 2019

liujing2 commented May 20, 2019 • edited Loading

sboeuf commented May 22, 2019

jiangliu commented May 27, 2019

sameo commented May 28, 2019

sameo commented May 28, 2019

sameo commented May 28, 2019

jiangliu commented May 28, 2019

sameo commented May 28, 2019

sboeuf commented May 28, 2019

jiangliu commented May 28, 2019

andreeaflorescu Mar 20, 2019 •

edited

Loading

liujing2 Apr 16, 2019 •

edited

Loading

liujing2 Apr 16, 2019 •

edited

Loading

liujing2 Apr 19, 2019 •

edited

Loading

liujing2 commented May 20, 2019 •

edited

Loading