Also make SetUserInfo idempotent

Signed-off-by: mulhern <[email protected]>
stratis-storage · Oct 7, 2019 · 5e4ddf6 · 5e4ddf6
1 parent da44b9e
commit 5e4ddf6
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 26 deletions.
diff --git a/src/dbus_api/blockdev.rs b/src/dbus_api/blockdev.rs
@@ -22,7 +22,7 @@ use crate::{
             msg_code_ok, msg_string_ok,
         },
     },
-    engine::{BlockDev, BlockDevTier, MaybeDbusPath},
+    engine::{BlockDev, BlockDevTier, DevUuid, MaybeDbusPath, RenameAction},
 };
 
 pub fn create_dbus_blockdev<'a>(
@@ -36,7 +36,11 @@ pub fn create_dbus_blockdev<'a>(
     let set_userid_method = f
         .method("SetUserInfo", (), set_user_info)
         .in_arg(("id", "s"))
-        .out_arg(("changed", "b"))
+        // b: false if no change to the user info
+        // s: UUID of the changed device
+        //
+        // Rust representation: (bool, String)
+        .out_arg(("changed", "(bs)"))
         .out_arg(("return_code", "q"))
         .out_arg(("return_string", "s"));
 
@@ -131,7 +135,7 @@ fn set_user_info(m: &MethodInfo<MTFn<TData>, TData>) -> MethodResult {
     let dbus_context = m.tree.get_data();
     let object_path = m.path.get_name();
     let return_message = message.method_return();
-    let default_return = false;
+    let default_return = (false, uuid_to_string!(DevUuid::nil()));
 
     let blockdev_path = m
         .tree
@@ -148,7 +152,22 @@ fn set_user_info(m: &MethodInfo<MTFn<TData>, TData>) -> MethodResult {
     let result = pool.set_blockdev_user_info(&pool_name, blockdev_data.uuid, new_id);
 
     let msg = match result {
-        Ok(id_changed) => return_message.append3(id_changed, msg_code_ok(), msg_string_ok()),
+        Ok(RenameAction::NoSource) => {
+            let error_message = format!(
+                "pool doesn't know about block device {}",
+                blockdev_data.uuid
+            );
+            let (rc, rs) = (DbusErrorEnum::INTERNAL_ERROR as u16, error_message);
+            return_message.append3(default_return, rc, rs)
+        }
+        Ok(RenameAction::Renamed(uuid)) => return_message.append3(
+            (true, uuid_to_string!(uuid)),
+            msg_code_ok(),
+            msg_string_ok(),
+        ),
+        Ok(RenameAction::Identity) => {
+            return_message.append3(default_return, msg_code_ok(), msg_string_ok())
+        }
         Err(err) => {
             let (rc, rs) = engine_to_dbus_err_tuple(&err);
             return_message.append3(default_return, rc, rs)

diff --git a/src/engine/engine.rs b/src/engine/engine.rs
@@ -175,7 +175,7 @@ pub trait Pool: Debug {
         pool_name: &str,
         uuid: DevUuid,
         user_info: Option<&str>,
-    ) -> StratisResult<bool>;
+    ) -> StratisResult<RenameAction<DevUuid>>;
 
     /// The current state of the Pool.
     fn state(&self) -> PoolState;

diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs
@@ -282,16 +282,17 @@ impl Pool for SimPool {
         _pool_name: &str,
         uuid: DevUuid,
         user_info: Option<&str>,
-    ) -> StratisResult<bool> {
-        self.get_mut_blockdev_internal(uuid).map_or_else(
-            || {
-                Err(StratisError::Engine(
-                    ErrorEnum::NotFound,
-                    format!("No blockdev for uuid {} found", uuid),
-                ))
+    ) -> StratisResult<RenameAction<DevUuid>> {
+        Ok(self.get_mut_blockdev_internal(uuid).map_or_else(
+            || RenameAction::NoSource,
+            |(_, b)| {
+                if b.set_user_info(user_info) {
+                    RenameAction::Renamed(uuid)
+                } else {
+                    RenameAction::Identity
+                }
             },
-            |(_, b)| Ok(b.set_user_info(user_info)),
-        )
+        ))
     }
 
     fn state(&self) -> PoolState {

diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs
@@ -30,6 +30,7 @@ use crate::{
             names::{format_backstore_ids, CacheRole},
             serde_structs::{BackstoreSave, CapSave, Recordable},
         },
+        types::RenameAction,
         BlockDevTier, DevUuid, PoolUuid,
     },
     stratis::{ErrorEnum, StratisError, StratisResult},
@@ -556,15 +557,16 @@ impl Backstore {
         &mut self,
         uuid: DevUuid,
         user_info: Option<&str>,
-    ) -> StratisResult<bool> {
+    ) -> RenameAction<DevUuid> {
         self.get_mut_blockdev_by_uuid(uuid).map_or_else(
-            || {
-                Err(StratisError::Engine(
-                    ErrorEnum::NotFound,
-                    format!("No blockdev for uuid {} found", uuid),
-                ))
+            || RenameAction::NoSource,
+            |(_, b)| {
+                if b.set_user_info(user_info) {
+                    RenameAction::Renamed(uuid)
+                } else {
+                    RenameAction::Identity
+                }
             },
-            |(_, b)| Ok(b.set_user_info(user_info)),
         )
     }
 }

diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs
@@ -454,13 +454,12 @@ impl Pool for StratPool {
         pool_name: &str,
         uuid: DevUuid,
         user_info: Option<&str>,
-    ) -> StratisResult<bool> {
-        if self.backstore.set_blockdev_user_info(uuid, user_info)? {
+    ) -> StratisResult<RenameAction<DevUuid>> {
+        let result = self.backstore.set_blockdev_user_info(uuid, user_info);
+        if let RenameAction::Renamed(_) = result {
             self.write_metadata(pool_name)?;
-            Ok(true)
-        } else {
-            Ok(false)
         }
+        Ok(result)
     }
 
     fn state(&self) -> PoolState {