[storage] [vfs] Remove use of 'unsafe' meaning "user should not call"
In this case we've used unsafe to notate things that are unsafe for
users to call (for lack of trait "private" functions). This is
problematic (https://fxbug.dev/99061) so I'm removing the annotations
for now.
Change-Id: Id834cdd8c90fed338ff781a63017e3fd3055ba6a
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/679190
Commit-Queue: Aaron Drew <ripper@google.com>
Reviewed-by: Stephen Demos <sdemos@google.com>
diff --git a/src/lib/storage/vfs/rust/src/directory/helper.rs b/src/lib/storage/vfs/rust/src/directory/helper.rs
index 015600b..565e02a 100644
--- a/src/lib/storage/vfs/rust/src/directory/helper.rs
+++ b/src/lib/storage/vfs/rust/src/directory/helper.rs
@@ -16,7 +16,6 @@
std::sync::Arc,
};
-#[allow(clippy::missing_safety_doc)] // TODO(fxbug.dev/99061)
/// `DirectlyMutable` is a superset of `MutableDirectory` which also allows server-side management
/// of directory entries (via `add_entry` and `remove_entry`). It also provides `rename_from` and
/// `rename_to` to support global ordering in rename operations. You may wish to use
@@ -85,21 +84,23 @@
/// Renaming needs to be atomic, even accross two distinct directories. So we need a special
/// API to handle that.
///
- /// As two distinc directories may mean two mutexes to lock, using it correctly is non-trivial.
+ /// As two distinct directories may mean two mutexes to lock, using it correctly is non-trivial.
/// In order to avoid a deadlock, we need to decide on a global ordering for the locks.
/// `rename_from` and [`Self::rename_to`] are used depending on the relative order of the two
/// directories involved in the operation.
///
- /// This method is `unsafe`, as `rename_from` and [`Self::rename_to`] should only be called by
- /// the [`crate::filesystem::FilesystemRename::rename()`], which will establish the global order
- /// and will call proper method. It should reduce the chances one will use this API
- /// incorrectly.
- ///
/// Implementations are expected to lock this directory, check that the entry exists and pass a
/// reference to the entry to the `to` callback. Only if the `to` callback succeed, should the
/// entry be removed from the current directory. This will garantee atomic rename from the
/// standpoint of the client.
- unsafe fn rename_from(
+ ///
+ /// # Safety
+ ///
+ /// This should only be called by the [`crate::filesystem::FilesystemRename::rename()`], which
+ /// will establish the global order and will call proper method.
+ ///
+ /// See fxbug.dev/99061: unsafe is not for visibility reduction.
+ fn rename_from(
&self,
src: String,
to: Box<dyn FnOnce(Arc<dyn DirectoryEntry>) -> Result<(), Status>>,
@@ -113,7 +114,14 @@
/// Implementations are expected to lock this dirctory, check if they can accept an entry named
/// `dst` (in case there might be any restrictions), then call the `from` callback to obtain a
/// new entry which must be added into the current directory with no errors.
- unsafe fn rename_to(
+ ///
+ /// # Safety
+ ///
+ /// This should only be called by the [`crate::filesystem::FilesystemRename::rename()`], which
+ /// will establish the global order and will call proper method.
+ ///
+ /// See fxbug.dev/99061: unsafe is not for visibility reduction.
+ fn rename_to(
&self,
dst: String,
from: Box<dyn FnOnce() -> Result<Arc<dyn DirectoryEntry>, Status>>,
diff --git a/src/lib/storage/vfs/rust/src/directory/simple.rs b/src/lib/storage/vfs/rust/src/directory/simple.rs
index 9a35bbe..57607be 100644
--- a/src/lib/storage/vfs/rust/src/directory/simple.rs
+++ b/src/lib/storage/vfs/rust/src/directory/simple.rs
@@ -403,7 +403,7 @@
}
}
- unsafe fn rename_from(
+ fn rename_from(
&self,
src: String,
to: Box<dyn FnOnce(Arc<dyn DirectoryEntry>) -> Result<(), Status>>,
@@ -429,7 +429,7 @@
Ok(())
}
- unsafe fn rename_to(
+ fn rename_to(
&self,
dst: String,
from: Box<dyn FnOnce() -> Result<Arc<dyn DirectoryEntry>, Status>>,
diff --git a/src/lib/storage/vfs/rust/src/filesystem/simple.rs b/src/lib/storage/vfs/rust/src/filesystem/simple.rs
index 3059fe9..b8077f9 100644
--- a/src/lib/storage/vfs/rust/src/filesystem/simple.rs
+++ b/src/lib/storage/vfs/rust/src/filesystem/simple.rs
@@ -44,34 +44,26 @@
let dst_order = dst_parent.as_ref() as *const dyn DirectlyMutable as *const usize as usize;
if src_order < dst_order {
- // `unsafe` here indicates that we have checked the global order for the locks for
+ // We must ensure that we have checked the global order for the locks for
// `src_parent` and `dst_parent` and we are calling `rename_from` as `src_parent` has a
// smaller memory address than the `dst_parent`.
- unsafe {
- src_parent.clone().rename_from(
- src.into_string(),
- Box::new(move |entry| {
- dst_parent.add_entry_impl(dst.into_string(), entry, true)
- }),
- )
- }
+ src_parent.clone().rename_from(
+ src.into_string(),
+ Box::new(move |entry| dst_parent.add_entry_impl(dst.into_string(), entry, true)),
+ )
} else if src_order == dst_order {
src_parent.rename_within(src.into_string(), dst.into_string())
} else {
- // `unsafe` here indicates that we have checked the global order for the locks for
+ // We must ensure that we have checked the global order for the locks for
// `src_parent` and `dst_parent` and we are calling `rename_to` as `dst_parent` has a
// smaller memory address than the `src_parent`.
- unsafe {
- dst_parent.rename_to(
- dst.into_string(),
- Box::new(move || {
- match src_parent.remove_entry_impl(src.into_string(), false)? {
- None => Err(Status::NOT_FOUND),
- Some(entry) => Ok(entry),
- }
- }),
- )
- }
+ dst_parent.rename_to(
+ dst.into_string(),
+ Box::new(move || match src_parent.remove_entry_impl(src.into_string(), false)? {
+ None => Err(Status::NOT_FOUND),
+ Some(entry) => Ok(entry),
+ }),
+ )
}
}
}
