Skip to content

Replace ByteValued with zerocopy#374

Open
arctic-alpaca wants to merge 5 commits intorust-vmm:mainfrom
arctic-alpaca:zerocopy-2
Open

Replace ByteValued with zerocopy#374
arctic-alpaca wants to merge 5 commits intorust-vmm:mainfrom
arctic-alpaca:zerocopy-2

Conversation

@arctic-alpaca
Copy link
Copy Markdown

Summary of the PR

Implementing ByteValued is unsafe and requires care to not introduce undefined behaviour. The zerocopy crate provides derivable traits that check at compiletime whether the struct in question satisfies the safety requirements instead.

The safety documentation for the ByteValued trait states that a type T can only implement ByteValued if and only if it can be initialized by reading its contents from a byte array. This doesn't cover the ByteValued::as{_mut}_slice methods, which allow treating type T as slice of bytes. From this, the ByteValued invariants are captured only by requiring both, zerocopy::FromBytes and zerocopy::IntoBytes. Additionally, ByteValued::zeroed allows constructing a T with all bytes set to zero, which means zerocopy::FromZeroes is also required to map ByteValued to zerocopy traits.

All ByteValued bounds are replaced by the existing bounds Copy + Send + Sync and the new bounds FromBytes + IntoBytes + FromZeros. Additionally, the endian module is removed in favour of zerocopy's endian handling.

Fixes #246

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@arctic-alpaca arctic-alpaca marked this pull request as draft March 29, 2026 18:14
@arctic-alpaca arctic-alpaca force-pushed the zerocopy-2 branch 2 times, most recently from beed516 to 23caac5 Compare March 29, 2026 18:20
@arctic-alpaca arctic-alpaca marked this pull request as ready for review March 29, 2026 18:35
Copy link
Copy Markdown
Collaborator

@ShadowCurse ShadowCurse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Please add commit messages to simplify two unsafes with zerocopy and simplify two unsafes with zerocopy. Also maybe change commit titles to zerocopy: ..., but no hard push here.
@roypat this might be of interest to you (we still have an internal ticket with your name on it about this change)

P.S. the CI fails with coverage change https://buildkite.com/rust-vmm/vm-memory-ci/builds/1246#019d3ad3-8e00-442a-9be3-fb3511606b95/L96 so please add a commit with update for it.

CHANGELOG.md Outdated

## Removed

- [...] Removed `ByteValued` trait and `endian` module in favor of the `zerocopy` crate.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can fill the [...] now.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

The zerocopy crate provides the same functionality.

Signed-off-by: Julian Schindel <mail@arctic-alpaca.de>
The safety documentation for the `ByteValued` trait states that a type
`T` can only implement `ByteValued` if and only if it can be initialized
by reading its contents from a byte array. This doesn't cover the
`ByteValued::as{_mut}_slice` methods, which allow treating type `T` as
slice of bytes. From this, the `ByteValued` invariants are captured only
by requiring both, `zerocopy::FromBytes` and `zerocopy::IntoBytes`.
Additionally, `ByteValued::zeroed` allows constructing a `T` with all
bytes set to zero, which means `zerocopy::FromZeroes` is also required
to map `ByteValued` to `zerocopy` traits.

All `ByteValued` bounds are replaced by the existing bounds
`Copy + Send + Sync` and the new bounds
`FromBytes + IntoBytes + FromZeros`.

Signed-off-by: Julian Schindel <mail@arctic-alpaca.de>
Only creates a (mutable) slice from a pointer instead of creating a
reference to a type `T`.
Moves alignement and size checks to `zerocopy`. 

Signed-off-by: Julian Schindel <mail@arctic-alpaca.de>
Removes mentions of `ByteValued` and updates the changelog.

Signed-off-by: Julian Schindel <mail@arctic-alpaca.de>
Adapts expected coverage to code change from `ByteValued` removal.

Signed-off-by: Julian Schindel <mail@arctic-alpaca.de>
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.

Introduce custom derive for ByteValued/replace ByteValued with zerocopy

2 participants