vmm-sys-util is currently licensed as "Apache-2.0 AND BSD-3-Clause", as opposed to "Apache-2.0 OR BSD-3-Clause", which appears to be intentional (cf. commit 04c2b8e).

IIUC, this precludes GPLv2 / GPLv2+ / LGPLv2.1 / LGPLv2.1+ projects from using this crate or any others that depend on it, which seems especially serious since it is a fairly fundamental building block.

Also, it seems there currently exist several crates in the rust-vmm project that depend on vmm-sys-util but are licensed in an (AFAICT) incompatible way. For instance, these dependent crates are "Apache-2.0 OR BSD-3-Clause" (not a comprehensive list):

• event-manager
• mshv-bindings
• mshv-ioctls
• vfio-bindings
• vfio-ioctls
• vhost
• vhost-device-gpio
• vhost-device-i2c
• vhost-device-rng
• virtio-queue

And these are "Apache-2.0 OR MIT":

• kvm-ioctls
• virtio-blk

Does this warrant any change to how vmm-sys-util is licensed? In particular, would relicensing be a possibility, perhaps as "Apache-2.0 OR BSD-3-Clause"?

Missing requirements for publishing to crates.io:

• Add #![deny(missing_docs)] to src/lib.rs
• In crate documentation. To check the documentation in HTML version run cargo doc --open
• Format the documentation with common sections like #Examples, #Arguments and Errors where applicable
• Update README.md
• Switch to rust-vmm-ci #9
• Check coverage of project and add more unit tests (if needed).
• Add CODEOWNERS
• Update crate metadata #13

SIGRTMIN() and SIGRTMAX() methods, which returns base signums for registering real-time signals are private, inside vmm-sys-util crate. It would be beneficial if these methods are public, to be consumed as well when signal handlers are registered with methods from this crate.

There are a few things missing from Cargo.toml. Some of them are required for publishing (like description), while others are nice to have.

• description
• repository
• readme
• keywords (this is making the package easy to find on crates.io)

For reference, you can check the Publishing a Crate to Crates.io.

I just noticed that vmm-sys-utils is not testing the musl build. There are also some other tests missing:

• build with musl on x86 and arm
• unit tests with musl on x86 and arm
• warning checks on x86 and arm

For examples you can check the kvm-ioctls pipeline.

I am looking in the signal.rs and I do not understand why real-time signals are for_vcpu signals, knowing that register_signal_handler function [0] receives the argument for_vcpu, which is registering a real-time signal with a signum equal to SIGRTMIN + num, when for_vcpu is true.

Why is semantically different how standard signals and real-time signals handlers are registered?
Couldn't we change the for_vcpu parameter name, of register_signal_handler function, to is_rtsig, lets say, and not lose any information about how the function should be used?

[0]

The syslog implementation and comments need to be cleaned up. There are a lot of "legacy" comments, TODOs and links added by the CrosVM team that are related to CrosVM and should not be in vmm-sys-util.

• vmm-sys-util/src/syslog.rs

  Line 164 in b546b9d

  // Uses libc's openlog function to get a socket to the syslogger. By getting the socket this way, as

• vmm-sys-util/src/syslog.rs

  Line 170 in b546b9d

  // TODO(zachr): Once https://android-review.googlesource.com/470998 lands, there won't be any

I also believe that the code in syslog is too complicated for no good reasons. While this makes sense in the CrosVM context, I think we should find a better workaround as I consider it to be relatively unsafe. Specifically the part that "guesses" that syslog is the first opened fd.

There are some things that can be simplified, the issue needs to be better scoped.

as_path() returns the path to the file if the Tempfile object that is wrapping the file is still in scope. If we remove the file by just calling remove(), as_path() can still be used to return the path to the file (even though that path doesn’t point at an existing entity anymore).

The documentation for as_path() is not very explicit about this and could be improved.

Most tests in the syslog are only calling functions/macros without actually checking that the functions/macros behaved as expected.

We should add checks for the expected entries in the syslog files.

SyscallReturnCode wraps only a c_int return value from a syscall.
While generally ok, there are syscalls that return other types. For example, mmap returns a void*, which is not representable by a c_int on 64-bit systems.

One option for fixing this would be to make SyscallReturnCode generic, wrapping a value of type T.
Another option would be to simply use c_long for this, since a 64-bit int can also be used to represent the 32-bit counterpart.

The released version the crate (v0.9.0) breaks when targeting Android even though support was added in #162; could it be upgraded?

In src/linux/epoll.rs the following function is defined:

pub fn wait(
    &self,
    max_events: usize,
    timeout: i32,
    events: &mut [EpollEvent],
) -> io::Result<usize> {
    // Safe because we give a valid epoll file descriptor and an array of epoll_event structures
    // that will be modified by the kernel to indicate information about the subset of file
    // descriptors in the interest list. We also check the return value.
    let events_count = SyscallReturnCode(unsafe {
        epoll_wait(
            self.epoll_fd,
            events.as_mut_ptr() as *mut epoll_event,
            max_events as i32,
            timeout,
        )
    })
    .into_result()? as usize;

    Ok(events_count)
}

It appears this is unsound, because there is no check that max_events is not greater than the size of the events slice. For example: if a call to wait is made wait(128, -1, some_slice) where some_slice is only large enough to hold 64 elements, then the kernel will write past the end of this slice leading to UB.

This function should therefore be marked unsafe or have an assert!(events.len() >= max_events) before the unsafe call to epoll_wait.

Hi!

This code has src/metric.rs, which unconditionally uses std::sync::atomic::AtomicU64. Quite some platforms don't have 64-bit atomics with current rust, namely it is armel, mipsel, powerpc and x32 (heh). Maybe the metric module can be guarded with the right #[cfg target_has_atomic] ?

Trying to package some stuff for Debian, this crate come as a dependency...

Thanks!

Replace fandree/rust-vmm-dev with rustvmm/dev:v1 in Buildkite pipeline.

dev:v1 has exactly the same packages and rust version as fandree/rust-vmm-dev so it should be a seamless change.

fam module is difficult to follow and might benefit from a better and more intuitive design.

Clone implementation for FamStructWrapper keeps only the number of entries and the entries array members. This implementation should be either removed or modified so that all fields are cloned.

The new function in FamStructWrapper is not validating that num_elements is less than the maximum capacity that the type supports (as defined in FamStruct::max_len), and thus might result in allocating more elements than it was initially intended.

A change in FamStructWrapper requires changes in kvm-bindings as well, so we might want to fix the vmm-sys-util dependency in kvm-bindings before doing a release. It might be better to remove the vmm-sys-util dependency from kvm-bindings (as this change does not pertain to the scope of kvm-bindings), and implement FamStruct directly in kvm-ioctls where it is actually needed.

Needed by: rust-vmm/event-manager#15

The following things need to be tested:

• Default implementation for EpollEvents
• PollToken implementation for u8, u16, u64.

Also the error cases are not tested, we should check if it is feasible to test the error cases as well.

Use rust-vmm-ci as a gitsubmodule to test the code. For more details about how to make the switch, please check the rust-vmm-ci readme

The other repositories are using a CHANGELOG.md format with the following sections: added/removed/changed/fixed. We should adopt it in this repo as well.

While working on the virtio block discard/write_zeroes implementation, I noticed that write_zeroes() is using fallocate with FALLOC_FL_PUNCH_HOLE mode instead of FALLOC_FL_ZERO_RANGE. From the virtio specification
it makes sense to use FALLOC_FL_PUNCH_HOLE for write zeroes request only if unmap bit is also set, otherwise the zeroed region shouldn't be deallocated.

I think it would make sense to either update write_zeroes() with FALLOC_FL_ZERO_RANGE or add an extra parameter to that function (mode for example) which can be any FallocateMode variant.

Moreover, we can completely switch to the latest crosvm write_zeroes which is using FALLOC_FL_ZERO_RANGE for File and adds a new trait WriteZeroesAt which lets you give the offset in file instead of using the current cursor. It also allows doing custom implementations for write_zeroes() (which might be useful for example for qcow file wrappers, example here).

Any thoughts / suggestions?

Is there a reason errno::Error is used instead of std::io::Error?

I've created a branch replacing errno::Error with io::Error. The main changes were:
replacing errno::Error::new() with io::Error::from_raw_os_error() and
replacing errno::Error::last() with errno::Error::last_os_error()

The only major change is that io::Error does not implement PartialEq so either ErrorKind or raw_os_error() needs to be used to compare errors.

This is a breaking change for consumers of this crate, but io::Error provides all the same functionality with different method names, so upgrading should be straightforward.

Tried to build vmm-sys-utils with musl and it fails:

   Compiling libc v0.2.58
   Compiling vmm-sys-util v0.1.0 (/home/local/ANT/fandree/sources/work/rust-vmm/vmm-sys-util)
error[E0308]: mismatched types
   --> src/signal.rs:317:9
    |
316 |     fn pthread_handle(&self) -> pthread_t {
    |                                 --------- expected `*mut libc::c_void` because of return type
317 |         self.as_pthread_t()
    |         ^^^^^^^^^^^^^^^^^^^ expected *-ptr, found u64
    |
    = note: expected type `*mut libc::c_void`
               found type `u64`

error: aborting due to previous error

The signal module has a coverage of 31% (as of 31 July 2019).
Most of the functionality is untested such as:

• Display implementation (this should be optional)
• create_sigset
• get_blocked_signals
• block_signal
• unblock_signal
• clear_signal

Testing signals might be tricky in rust in the unit tests as all unit tests run in the same process. There are some mechanisms by which this problem can be avoided such as spawning a new process for tests that might kill the main process which runs the unit tests.

Error/Panic message: thread 'main' panicked at 'attempt to subtract with overflow'

For now, we are testing only the inc() method (and automatically add()) for Metric. We should add tests for count(), reset() and set() methods too.

For example, count() can be tested as follows:
assert_eq!(system_metrics.dog_metrics.bark.count(), 2); (at the end of test_main())

Just noticed that register_signal_handler from signal.rs:

To be specific, I am referring to a way to initialize the sa_mask field of a struct sigaction, when calling register_signal_handler. An example of my usecase is here [0].

Are you fond of a way to achieve this with the current implementation?
If it couldn't be achieved with the current implementation, I can gladly contribute with a PR.

[0] https://www.gnu.org/software/libc/manual/html_node/Blocking-for-Handler.html#Blocking-for-Handler.

It looks like the other rust-vmm crates are already using edition 2018. We should also switch this one and get rid of the annoying macro_use where possible.

The tests from tempdir are not covering the case when we remove a directory that contains other directories or files. We should add a test that checks this scenario too.

The FAM struct implementation is minimalist and only works for FAM structs that have 2 fields: the number of entries and the incomplete array type.

FAM structs by their definition just require that the last field in the structure is the incomplete array type, but it does not talk about the number of other fields.

We should extend this implementation to support other scenarios as well. Because of this miss, vfio-ioctls cannot use the FamStructWrapper code and needs to redefine how to work with FAM.

With Rust 1.39 warning checks failed because in src/unix/aio.rs we use the deprecated mem::uninitialized. This should be replaced with the safe version mem::MaybeUninit.

We provide an implementation of the Metric trait (which contains methods such as add() and inc()) only for AtomicU64. According to its documentation, in case of an integer overflow, we start over from 0.
We should document this in the AtomicU64 Metric implementation (for example we can add a comment to add() documentation).
We should also add a test for an integer overflow when using add().

Examples in the documentation should be clear and concise. Unnecessary boilerplate should be hidden from the generated code snippets.

The unit tests from write_zeroes don't cover the case when punch_hole() call from write_zeroes() returns an error: https://github.com/rust-vmm/vmm-sys-util/blob/master/src/linux/write_zeroes.rs#L52-L64.

punch_hole() fails if, for example, libc::fallocate64 from here fails. We can test the aforementioned path if we make fallocate fail, by choosing to simulate some error from here.
Example: pass 0 length to write_zeroes() (this is a pretty dumb scenario, offset+len exceeds the maximum file size error could be more interesting).

@sameo @andreeaflorescu @jiangliu
There are many const variables copied from linux kernel that don't have a proper documentation.
Does anyone have some suggestion?

The failures happen because all tests in one crate are running in a single process. This means we call init() multiple time in the same process which is results in undefined behaviour.

Fixing this is as easy as having just one unit test for the syslog module.

CpuId and Msrs will need to be serialized for firecracker-microvm/firecracker#1184. The FAM wrappers need to be (conditionally) enhanced with serde support.

SSIA 😄

Two options worth considering:

1. Add support for temporary file operations, like read, write, seek, but also support for file permissions (open TempFile with specific file permissions). Some progress was done in this direction: #60.

2. When creating a temporary file/directory, we should unlink it immediately, then return a File that represent the handle for that unliked file/directory, that we're gonna operate with, and leave altogether the TempFile/TempDir structs as builders for these temporary files. This would simplify a lot the implementation, and wouldn't need any support for file operations and permissions.

I don't know if it's a project goal for unit tests to work on any platform, but I would imagine it's better if they run on developer machines.

I get the following output when running (macOS Big Sur -- on M1 aarch64 mac mini)

running 38 tests
test errno::tests::test_display ... ok
test fam::tests::test_mem_allocator_len ... ok
test fam::tests::test_raw_content ... ok
test fam::tests::test_new ... ok
test fam::tests::test_entries_slice ... ok
test errno::tests::test_errno ... ok
test fam::tests::test_push ... ok
test fam::tests::test_from_entries ... ok
test fam::tests::test_clone ... ok
test fam::tests::test_reserve ... ok
test fam::tests::test_partial_eq ... ok
test fam::tests::test_set_len ... ok
test fam::tests::test_ser_deser ... ok
test fam::tests::test_wrapper_len ... ok
test fam::tests::test_retain ... ok
test rand::tests::test_rand_alphanumerics_impl ... ok
test rand::tests::test_rand_alphanumerics ... ok
test metric::tests::test_main ... ok
test syscall::tests::test_syscall_ops ... ok
test rand::tests::test_xor_psuedo_rng_u8_alphas ... ok
test rand::tests::test_timestamp_cycles ... FAILED
test rand::tests::test_xor_psuedo_rng_u32 ... FAILED
test tempfile::tests::test_create_file_new ... ok
test tempfile::tests::test_into_file ... ok
test tempfile::tests::test_remove_file ... ok
test tempfile::tests::test_drop_file ... ok
test tempfile::tests::test_create_file_new_in ... ok
test unix::file_traits::tests::test_set_len ... ok
test unix::file_traits::tests::test_set_len_fails_when_file_not_opened_for_writing ... ok
test tempfile::tests::test_create_file_with_prefix ... ok
test unix::tempdir::tests::test_create_dir ... FAILED
test unix::tempdir::tests::test_create_dir_in ... FAILED
test unix::tempdir::tests::test_drop ... ok
test unix::tempdir::tests::test_create_dir_with_prefix ... ok
test unix::terminal::tests::test_a_non_tty ... ok
test unix::terminal::tests::test_a_tty ... ok
test unix::tempdir::tests::test_remove_dir ... ok
test unix::file_traits::tests::test_fsync ... ok

failures:

---- rand::tests::test_timestamp_cycles stdout ----
thread 'rand::tests::test_timestamp_cycles' panicked at 'assertion failed: timestamp_cycles() < timestamp_cycles()', src/rand.rs:96:13

---- rand::tests::test_xor_psuedo_rng_u32 stdout ----
thread 'rand::tests::test_xor_psuedo_rng_u32' panicked at 'assertion failed: `(left != right)`
  left: `3767613165`,
 right: `3767613165`', src/rand.rs:103:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- unix::tempdir::tests::test_create_dir stdout ----
thread 'unix::tempdir::tests::test_create_dir' panicked at 'assertion failed: path.starts_with(\"/tmp/\")', src/unix/tempdir.rs:149:9

---- unix::tempdir::tests::test_create_dir_in stdout ----
thread 'unix::tempdir::tests::test_create_dir_in' panicked at 'assertion failed: path.starts_with(\"/tmp/\")', src/unix/tempdir.rs:190:9


failures:
    rand::tests::test_timestamp_cycles
    rand::testerror: io error when listing tests: Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }
s::test_xor_psuedo_rng_u32
error: test failed, to rerun pass '--lib'

Please add a CODEOWNERS file with the details of the maintainers. Please use the following format as outlined in: https://help.github.com/en/articles/about-code-owners

Most projects can simply follow the wildcard syntax. e.g.

* @owner1 @owner2

Not only does this provide a way to see who is responsible or this repository they will also automatically be notified of incoming PR reviews.

Test signal::test_killing_thread fails when running in release mode with the following output:

error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/home/ANT.AMAZON.COM/lauralg/vmm-sys-util/target/release/deps/vmm_sys_util-fc40402ad0d0503d` (signal: 4, SIGILL: illegal instruction)

Currently, the tests from file_traits are testing only the happy paths.

We could easily test the error propagation too by checking for example that set_len fails if it is called for a file that was not opened for writing: https://docs.rs/ex/0.1.0/ex/fs/struct.File.html#errors.

I understand if you'd rather wait to fit in other changes, but there is an unreleased change on the master branch I'd like to use downstream. It wouldn't be the most exciting release 😅 but I'd appreciate it if/when it does happen. 🙂

All crates for rust-vmm should enable #![deny(missing_docs)] check in src/lib.rs.

The coverage as of 30 July 2019 is 67.9%.

The following functions and trait implementations are not tested:

• clear()
• FromRawFd
• IntoRawFd

Also, error cases are also not tested, but I am not sure how feasible it is to test them.

@jiangliu suggested that we need some expansion on current timer implementation. Could you kindly help tell some details e.g. which functionality do we need, so we can use this issue to track? @jiangliu

The methods which are used for getting base signum for real-time signals, namely SIGRTMIN() and SIGRTMAX(), from signal.rs, are private. It would be nice to expose them as public, with the advent of manually passing real-time signums to methods which register and validate signal handlers, like register_signal_handler and validate_signal_num, from signal.rs.