From 30e79de26fc98c400c0bc58be875d4b959cf1837 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Tue, 9 Jun 2026 17:05:34 +0100 Subject: [PATCH] block: qcow: Port RawFile to AlignedBuffer Replace the manual alloc_zeroed/dealloc and pread64/pwrite64 with use of the new AlignedBuffer structure. Assisted-by: Claude:Opus-4.6 Signed-off-by: Rob Bradford --- block/src/aligned_buffer.rs | 3 +- block/src/formats/qcow/internal/raw_file.rs | 299 +++++++++----------- 2 files changed, 129 insertions(+), 173 deletions(-) diff --git a/block/src/aligned_buffer.rs b/block/src/aligned_buffer.rs index 6370825b3..dbe8c6279 100644 --- a/block/src/aligned_buffer.rs +++ b/block/src/aligned_buffer.rs @@ -13,7 +13,6 @@ use std::{io, slice}; /// at `as_slice()`/`as_mut_slice()` (accounting for head padding when the /// requested offset is not alignment-aligned). The full aligned region is /// used internally for pread/pwrite via `FileExt`. -#[allow(dead_code)] pub(crate) struct AlignedBuffer { ptr: *mut u8, layout: Layout, @@ -23,7 +22,6 @@ pub(crate) struct AlignedBuffer { aligned_offset: u64, } -#[allow(dead_code)] impl AlignedBuffer { /// Create a new aligned buffer for I/O at `offset` of `len` bytes with /// the given `alignment` requirement. @@ -104,6 +102,7 @@ impl AlignedBuffer { } /// Read the full aligned region from `f` into this buffer. + #[cfg_attr(not(test), expect(dead_code))] pub fn read_exact_from(&mut self, f: &impl FileExt) -> io::Result<()> { let offset = self.aligned_offset; f.read_exact_at(self.full_mut_slice(), offset) diff --git a/block/src/formats/qcow/internal/raw_file.rs b/block/src/formats/qcow/internal/raw_file.rs index ed152b8bc..a04ecd34a 100644 --- a/block/src/formats/qcow/internal/raw_file.rs +++ b/block/src/formats/qcow/internal/raw_file.rs @@ -8,18 +8,18 @@ // // SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause -use std::alloc::{Layout, alloc_zeroed, dealloc}; use std::fs::{File, Metadata}; use std::io::{self, Read, Seek, SeekFrom, Write}; use std::os::fd::{AsFd, BorrowedFd}; use std::os::unix::fs::FileExt; use std::os::unix::io::{AsRawFd, RawFd}; -use std::{result, slice}; +use std::result; use vmm_sys_util::file_traits::FileSync; use vmm_sys_util::seek_hole::SeekHole; use vmm_sys_util::write_zeroes::{PunchHole, WriteZeroesAt}; +use crate::aligned_buffer::AlignedBuffer; use crate::{BlockBackend, query_device_size}; #[derive(Debug)] @@ -32,19 +32,12 @@ pub struct RawFile { const BLK_ALIGNMENTS: [usize; 2] = [512, 4096]; -fn is_valid_alignment(fd: RawFd, alignment: usize) -> bool { - let layout = Layout::from_size_align(alignment, alignment).unwrap(); - // SAFETY: layout has non-zero size - let ptr = unsafe { alloc_zeroed(layout) }; - assert!(!ptr.is_null()); - - // SAFETY: FFI call - let ret = unsafe { ::libc::pread(fd, ptr.cast(), alignment, alignment.try_into().unwrap()) }; - - // SAFETY: ptr was allocated by alloc_zeroed with layout - unsafe { dealloc(ptr, layout) }; - - ret >= 0 +fn is_valid_alignment(file: &File, alignment: usize) -> bool { + let mut abuf = match AlignedBuffer::new(alignment as u64, alignment, alignment) { + Ok(b) => b, + Err(_) => return false, + }; + abuf.read_from(file).is_ok() } impl RawFile { @@ -53,7 +46,7 @@ impl RawFile { let mut alignment = 0; if direct_io { for align in &BLK_ALIGNMENTS { - if is_valid_alignment(file.as_raw_fd(), *align) { + if is_valid_alignment(&file, *align) { alignment = *align; break; } @@ -67,16 +60,6 @@ impl RawFile { } } - fn round_up(&self, offset: u64) -> u64 { - let align: u64 = self.alignment.try_into().unwrap(); - offset.div_ceil(align) * align - } - - fn round_down(&self, offset: u64) -> u64 { - let align: u64 = self.alignment.try_into().unwrap(); - (offset / align) * align - } - fn is_aligned(&self, buf: &[u8]) -> bool { if self.alignment == 0 { return true; @@ -136,6 +119,10 @@ impl RawFile { impl Read for RawFile { fn read(&mut self, buf: &mut [u8]) -> io::Result { + if buf.is_empty() { + return Ok(0); + } + if self.is_aligned(buf) { match self.file.read(buf) { Ok(r) => { @@ -145,77 +132,22 @@ impl Read for RawFile { Err(e) => Err(e), } } else { - let rounded_pos: u64 = self.round_down(self.position); - let file_offset: usize = self - .position - .checked_sub(rounded_pos) - .unwrap() - .try_into() - .unwrap(); - let buf_len: usize = buf.len(); - let rounded_len: usize = self - .round_up( - file_offset - .checked_add(buf_len) - .unwrap() - .try_into() - .unwrap(), - ) - .try_into() - .unwrap(); - - let layout = Layout::from_size_align(rounded_len, self.alignment).unwrap(); - // SAFETY: layout has non-zero size - let tmp_ptr = unsafe { alloc_zeroed(layout) }; - if tmp_ptr.is_null() { - return Err(io::Error::last_os_error()); - } - - // SAFETY: tmp_ptr is valid and at least rounded_len long - let tmp_buf = unsafe { slice::from_raw_parts_mut(tmp_ptr, rounded_len) }; - - // This can eventually replaced with read_at once its interface - // has been stabilized. - // SAFETY: FFI call. All parameters are valid. - let ret = unsafe { - ::libc::pread64( - self.file.as_raw_fd(), - tmp_buf.as_mut_ptr().cast(), - tmp_buf.len(), - rounded_pos.try_into().unwrap(), - ) - }; - if ret < 0 { - // SAFETY: tmp_ptr was allocated by alloc_zeroed with layout - unsafe { dealloc(tmp_ptr, layout) }; - return Err(io::Error::last_os_error()); - } - - let read: usize = ret.try_into().unwrap(); - if read < file_offset { - // SAFETY: tmp_ptr was allocated by alloc_zeroed with layout - unsafe { dealloc(tmp_ptr, layout) }; - return Ok(0); - } - - let mut to_copy = read - file_offset; - if to_copy > buf_len { - to_copy = buf_len; - } - - buf.copy_from_slice(&tmp_buf[file_offset..(file_offset + buf_len)]); - // SAFETY: tmp_ptr was allocated by alloc_zeroed with layout - unsafe { dealloc(tmp_ptr, layout) }; - - self.seek(SeekFrom::Current(to_copy.try_into().unwrap())) - .unwrap(); - Ok(to_copy) + let buf_len = buf.len(); + let mut abuf = AlignedBuffer::new(self.position, buf_len, self.alignment)?; + let bytes_read = abuf.read_from(&self.file)?; + buf[..bytes_read].copy_from_slice(&abuf.as_slice()[..bytes_read]); + self.seek(SeekFrom::Current(bytes_read.try_into().unwrap()))?; + Ok(bytes_read) } } } impl Write for RawFile { fn write(&mut self, buf: &[u8]) -> io::Result { + if buf.is_empty() { + return Ok(0); + } + if self.is_aligned(buf) { match self.file.write(buf) { Ok(r) => { @@ -225,86 +157,13 @@ impl Write for RawFile { Err(e) => Err(e), } } else { - let rounded_pos: u64 = self.round_down(self.position); - let file_offset: usize = self - .position - .checked_sub(rounded_pos) - .unwrap() - .try_into() - .unwrap(); - let buf_len: usize = buf.len(); - let rounded_len: usize = self - .round_up( - file_offset - .checked_add(buf_len) - .unwrap() - .try_into() - .unwrap(), - ) - .try_into() - .unwrap(); - - let layout = Layout::from_size_align(rounded_len, self.alignment).unwrap(); - // SAFETY: layout has non-zero size - let tmp_ptr = unsafe { alloc_zeroed(layout) }; - if tmp_ptr.is_null() { - return Err(io::Error::last_os_error()); - } - - // SAFETY: tmp_ptr is at least rounded_len long - let tmp_buf = unsafe { slice::from_raw_parts_mut(tmp_ptr, rounded_len) }; - - // This can eventually replaced with read_at once its interface - // has been stabilized. - // SAFETY: FFI call - let ret = unsafe { - ::libc::pread64( - self.file.as_raw_fd(), - tmp_buf.as_mut_ptr().cast(), - tmp_buf.len(), - rounded_pos.try_into().unwrap(), - ) - }; - if ret < 0 { - // SAFETY: tmp_ptr was allocated by alloc_zeroed with layout - unsafe { dealloc(tmp_ptr, layout) }; - return Err(io::Error::last_os_error()); - } - - tmp_buf[file_offset..(file_offset + buf_len)].copy_from_slice(buf); - - // This can eventually replaced with write_at once its interface - // has been stabilized. - // SAFETY: FFI call - let ret = unsafe { - ::libc::pwrite64( - self.file.as_raw_fd(), - tmp_buf.as_ptr().cast(), - tmp_buf.len(), - rounded_pos.try_into().unwrap(), - ) - }; - - // SAFETY: tmp_ptr was allocated by alloc_zeroed with layout - unsafe { dealloc(tmp_ptr, layout) }; - - if ret < 0 { - return Err(io::Error::last_os_error()); - } - - let written: usize = ret.try_into().unwrap(); - if written < file_offset { - Ok(0) - } else { - let mut to_seek = written - file_offset; - if to_seek > buf_len { - to_seek = buf_len; - } - - self.seek(SeekFrom::Current(to_seek.try_into().unwrap())) - .unwrap(); - Ok(to_seek) - } + let buf_len = buf.len(); + let mut abuf = AlignedBuffer::new(self.position, buf_len, self.alignment)?; + abuf.read_from(&self.file)?; + abuf.as_mut_slice().copy_from_slice(buf); + abuf.write_to(&self.file)?; + self.seek(SeekFrom::Current(buf_len.try_into().unwrap()))?; + Ok(buf_len) } } @@ -415,3 +274,101 @@ impl FileExt for RawFile { self.file.write_at(buf, offset) } } + +#[cfg(test)] +mod tests { + use std::fs::File; + use std::io::{Read, Seek, SeekFrom, Write}; + use std::os::unix::fs::FileExt; + + use vmm_sys_util::tempfile::TempFile; + + use super::*; + + const TEST_ALIGNMENT: usize = 512; + + fn create_pattern_file(size: usize) -> TempFile { + let tf = TempFile::new().unwrap(); + let pattern: Vec = (0..size).map(|i| (i % 251) as u8).collect(); + tf.as_file().write_all(&pattern).unwrap(); + tf.as_file().sync_all().unwrap(); + tf + } + + fn raw_with_alignment(file: File) -> RawFile { + RawFile { + file, + alignment: TEST_ALIGNMENT, + position: 0, + direct_io: true, + } + } + + #[test] + fn test_alignment_probe_accepts_short_read() { + let tf = create_pattern_file(1); + + assert!(is_valid_alignment(tf.as_file(), TEST_ALIGNMENT)); + } + + #[test] + fn test_unaligned_read_returns_short_read_at_eof() { + let file_size = 100usize; + let tf = create_pattern_file(file_size); + let mut raw = raw_with_alignment(tf.as_file().try_clone().unwrap()); + raw.seek(SeekFrom::Start(10)).unwrap(); + + let mut buf = vec![0u8; 200]; + let bytes_read = raw.read(&mut buf).unwrap(); + + let expected: Vec = (10..file_size).map(|i| (i % 251) as u8).collect(); + assert_eq!(bytes_read, expected.len()); + assert_eq!(&buf[..bytes_read], &expected[..]); + assert_eq!(raw.position, file_size as u64); + } + + #[test] + fn test_unaligned_read_beyond_eof_returns_zero() { + let tf = create_pattern_file(100); + let mut raw = raw_with_alignment(tf.as_file().try_clone().unwrap()); + raw.seek(SeekFrom::Start(200)).unwrap(); + + let mut buf = vec![0u8; 16]; + let bytes_read = raw.read(&mut buf).unwrap(); + + assert_eq!(bytes_read, 0); + assert_eq!(raw.position, 200); + } + + #[test] + fn test_unaligned_write_extends_at_eof() { + let file_size = 100usize; + let tf = create_pattern_file(file_size); + let mut raw = raw_with_alignment(tf.as_file().try_clone().unwrap()); + raw.seek(SeekFrom::Start(file_size as u64)).unwrap(); + + let data = b"xyz"; + let bytes_written = raw.write(data).unwrap(); + + assert_eq!(bytes_written, data.len()); + assert_eq!(raw.position, (file_size + data.len()) as u64); + + let mut readback = vec![0u8; file_size + data.len()]; + tf.as_file().read_exact_at(&mut readback, 0).unwrap(); + let expected_prefix: Vec = (0..file_size).map(|i| (i % 251) as u8).collect(); + assert_eq!(&readback[..file_size], &expected_prefix[..]); + assert_eq!(&readback[file_size..], data); + } + + #[test] + fn test_empty_unaligned_io_is_noop() { + let tf = create_pattern_file(100); + let mut raw = raw_with_alignment(tf.as_file().try_clone().unwrap()); + raw.seek(SeekFrom::Start(1)).unwrap(); + + let mut read_buf = []; + assert_eq!(raw.read(&mut read_buf).unwrap(), 0); + assert_eq!(raw.write(&[]).unwrap(), 0); + assert_eq!(raw.position, 1); + } +}