Use NonNull<u8> in MmapRegionBuilder for saving space

rust-vmm / vm-memory

Virtual machine's guest memory crate

Apache License 2.0

305 stars 98 forks source link

Use NonNull<u8> in MmapRegionBuilder for saving space #224

Open FuuuOverclocking opened 1 year ago

FuuuOverclocking commented 1 year ago

Summary of the PR

Updates the raw_ptr field in MmapRegionBuilder (mmap_unix.rs) from Option<*mut u8> to Option<NonNull<u8>>. This modification saves 8 bytes on x86-64 systems, for example.

The public API and functionality remain unchanged.

Requirements

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

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

FuuuOverclocking commented 1 year ago

This commit caused the test to fail due to fn test_mmap_region_build_raw() (vm_memory::mmap_unix::tests) as addr is 0 in this test.

I'm wondering, is addr being 0 a valid parameter? Since the addr here is an HVA, it seems inappropriate to set it to 0. Instead, it could be set to, for example, 40960.

andreeaflorescu commented 1 year ago

@FuuuOverclocking what is the reason for this PR? 8 bytes in the great scheme of things does not feel like a lot.

FuuuOverclocking commented 1 year ago

In Rust, using Option<NonNull<u8>> is a common pattern when both Option and pointer are needed.

epilys commented 11 months ago

@FuuuOverclocking Hello, I think this PR should also wrap MmapRegion::addr in NonNull<_> as well. Would you be interested in rebasing and making this change?

roypat commented 10 months ago

This seems reasonable to me, I don't think there's any use case in mapping in the zero page, and Option<NonNull<u8>> is indeed the more rust-y approach.

I also agree with @epilys that if we do this, we should probably be consistent with it, and use NonNull<u8> everywhere (but in this case, this probably will need a changelog entry).

Also, it currently seems like unittests are failing, could you look into that? :o