review feedback

oxidecomputer · Dec 13, 2024 · b650639 · b650639
1 parent feb1bdc
commit b650639
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 46 deletions.
diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs
@@ -390,44 +390,32 @@ pub enum WhichDatasets {
     SelfAndChildren,
 }
 
-// A helper structure for gathering all possible key/value pairs to pass to "zfs
-// set". This helps callers minimize the number of calls to "zfs set" they need
-// to make.
-struct PropertySetter {
-    props: Vec<(&'static str, String)>,
-}
-
-impl PropertySetter {
-    fn new() -> Self {
-        PropertySetter { props: Vec::new() }
-    }
-
-    fn add_size_details(&mut self, details: SizeDetails) -> &mut Self {
-        let SizeDetails { quota, reservation, compression } = details;
+fn build_zfs_set_key_value_pairs(
+    size_details: Option<SizeDetails>,
+    dataset_id: Option<DatasetUuid>,
+) -> Vec<(&'static str, String)> {
+    let mut props = Vec::new();
+    if let Some(SizeDetails { quota, reservation, compression }) = size_details
+    {
         let quota = quota
             .map(|q| q.to_bytes().to_string())
             .unwrap_or_else(|| String::from("none"));
-        self.props.push(("quota", quota));
+        props.push(("quota", quota));
 
         let reservation = reservation
             .map(|r| r.to_bytes().to_string())
             .unwrap_or_else(|| String::from("none"));
-        self.props.push(("reservation", reservation));
+        props.push(("reservation", reservation));
 
         let compression = compression.to_string();
-        self.props.push(("compression", compression));
-
-        self
+        props.push(("compression", compression));
     }
 
-    fn add_id(&mut self, id: DatasetUuid) -> &mut Self {
-        self.props.push(("oxide:uuid", id.to_string()));
-        self
+    if let Some(id) = dataset_id {
+        props.push(("oxide:uuid", id.to_string()));
     }
 
-    fn as_vec(&self) -> &Vec<(&str, String)> {
-        &self.props
-    }
+    props
 }
 
 impl Zfs {
@@ -559,21 +547,14 @@ impl Zfs {
     ) -> Result<(), EnsureFilesystemError> {
         let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?;
 
-        let mut props = PropertySetter::new();
-        if let Some(size_details) = size_details {
-            props.add_size_details(size_details);
-        }
-        if let Some(id) = id {
-            props.add_id(id);
-        }
-
+        let props = build_zfs_set_key_value_pairs(size_details, id);
         if exists {
-            Self::set_values(name, props.as_vec().as_slice()).map_err(
-                |err| EnsureFilesystemError {
+            Self::set_values(name, props.as_slice()).map_err(|err| {
+                EnsureFilesystemError {
                     name: name.to_string(),
                     err: err.err.into(),
-                },
-            )?;
+                }
+            })?;
 
             if encryption_details.is_none() {
                 // If the dataset exists, we're done. Unencrypted datasets are
@@ -643,7 +624,7 @@ impl Zfs {
             })?;
         }
 
-        Self::set_values(name, props.as_vec().as_slice()).map_err(|err| {
+        Self::set_values(name, props.as_slice()).map_err(|err| {
             EnsureFilesystemError {
                 name: name.to_string(),
                 err: err.err.into(),

diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs
@@ -998,22 +998,23 @@ impl StorageManager {
             return Ok(false);
         };
 
+        if old_id != config.id {
+            return Err(Error::UuidMismatch {
+                name: config.name.full_name(),
+                old: old_id.into_untyped_uuid(),
+                new: config.id.into_untyped_uuid(),
+            });
+        }
+
         let old_props = match SharedDatasetConfig::try_from(old_dataset) {
             Ok(old_props) => old_props,
             Err(err) => {
-                warn!(log, "Failed to parse old properties"; "err" => ?err);
+                warn!(log, "Failed to parse old properties"; "err" => #%err);
                 return Ok(false);
             }
         };
 
         info!(log, "Parsed old dataset properties"; "props" => ?old_props);
-        if old_id != config.id {
-            return Err(Error::UuidMismatch {
-                name: config.name.full_name(),
-                old: old_id.into_untyped_uuid(),
-                new: config.id.into_untyped_uuid(),
-            });
-        }
         if old_props != config.inner {
             info!(
                 log,