Skip to content

Commit

Permalink
Review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
yellowhatter committed Jul 16, 2024
1 parent daf7d57 commit 44d00e1
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,7 @@ impl PosixShmProviderBackendBuilder {
size: usize,
alignment: AllocAlignment,
) -> Result<LayoutedPosixShmProviderBackendBuilder<MemoryLayout>, ZLayoutError> {
let layout = MemoryLayout::new(
size.try_into()
.map_err(|_| ZLayoutError::IncorrectLayoutArgs)?,
alignment,
)?;
let layout = MemoryLayout::new(size, alignment)?;
Ok(LayoutedPosixShmProviderBackendBuilder { layout })
}

Expand All @@ -102,11 +98,7 @@ impl PosixShmProviderBackendBuilder {
self,
size: usize,
) -> Result<LayoutedPosixShmProviderBackendBuilder<MemoryLayout>, ZLayoutError> {
let layout = MemoryLayout::new(
size.try_into()
.map_err(|_| ZLayoutError::IncorrectLayoutArgs)?,
AllocAlignment::default(),
)?;
let layout = MemoryLayout::new(size, AllocAlignment::default())?;
Ok(LayoutedPosixShmProviderBackendBuilder { layout })
}
}
Expand Down
28 changes: 25 additions & 3 deletions commons/zenoh-shm/src/api/provider/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ impl Default for AllocAlignment {
}

impl AllocAlignment {
/// Try to create a new AllocAlignment from alignment representation in powers of 2.
///
/// # Errors
///
/// This function will return an error if provided alignment power cannot fit into usize.
#[zenoh_macros::unstable_doc]
pub fn new(pow: u8) -> Result<Self, ZLayoutError> {
match pow {
Expand All @@ -69,7 +74,7 @@ impl AllocAlignment {
/// Get alignment in normal units (bytes)
#[zenoh_macros::unstable_doc]
pub fn get_alignment_value(&self) -> NonZeroUsize {
// SAFETY: this is safe
// SAFETY: this is safe because we limit pow in new based on usize size
unsafe { NonZeroUsize::new_unchecked(1usize << self.pow) }
}

Expand All @@ -90,6 +95,12 @@ impl AllocAlignment {
let alignment = self.get_alignment_value();
match size.get() % alignment {
0 => size,
// SAFETY:
// This unsafe block is always safe:
// 1. 0 < remainder < alignment
// 2. because of 1, the value of (alignment.get() - remainder) is always > 0
// 3. because of 2, we add nonzero size to nonzero (alignment.get() - remainder) and it is always positive if no overflow
// 4. we make sure that there is no overflow condition in 3 by means of alignment limitation in `new` by limiting pow value
remainder => unsafe {
NonZeroUsize::new_unchecked(size.get() + (alignment.get() - remainder))
},
Expand All @@ -115,9 +126,20 @@ impl Display for MemoryLayout {
}

impl MemoryLayout {
/// Try to create a new memory layout
/// Try to create a new memory layout.
///
/// # Errors
///
/// This function will return an error if zero size have passed or if the provided size is not the mutiply of the alignment.

Check warning on line 133 in commons/zenoh-shm/src/api/provider/types.rs

View workflow job for this annotation

GitHub Actions / Typos Check

"mutiply" should be "multiply".
#[zenoh_macros::unstable_doc]
pub fn new(size: NonZeroUsize, alignment: AllocAlignment) -> Result<Self, ZLayoutError> {
pub fn new<T>(size: T, alignment: AllocAlignment) -> Result<Self, ZLayoutError>
where
T: TryInto<NonZeroUsize>,
{
let Ok(size) = size.try_into() else {
return Err(ZLayoutError::IncorrectLayoutArgs);
};

// size of an allocation must be a miltiple of it's alignment!
match size.get() % alignment.get_alignment_value() {
0 => Ok(Self { size, alignment }),
Expand Down
7 changes: 3 additions & 4 deletions commons/zenoh-shm/tests/posix_shm_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ fn posix_shm_provider_alloc() {
.res()
.expect("Error creating PosixShmProviderBackend!");

let layout = MemoryLayout::new(100.try_into().unwrap(), AllocAlignment::default()).unwrap();
let layout = MemoryLayout::new(100, AllocAlignment::default()).unwrap();

let _buf = backend
.alloc(&layout)
Expand All @@ -58,7 +58,7 @@ fn posix_shm_provider_open() {
.res()
.expect("Error creating PosixShmProviderBackend!");

let layout = MemoryLayout::new(100.try_into().unwrap(), AllocAlignment::default()).unwrap();
let layout = MemoryLayout::new(100, AllocAlignment::default()).unwrap();

let buf = backend
.alloc(&layout)
Expand All @@ -79,8 +79,7 @@ fn posix_shm_provider_allocator() {
.res()
.expect("Error creating PosixShmProviderBackend!");

let layout =
MemoryLayout::new(BUFFER_SIZE.try_into().unwrap(), AllocAlignment::default()).unwrap();
let layout = MemoryLayout::new(BUFFER_SIZE, AllocAlignment::default()).unwrap();

// exaust memory by allocating it all
let mut buffers = vec![];
Expand Down

0 comments on commit 44d00e1

Please sign in to comment.