[pbs-devel] [PATCH proxmox-backup 1/5] tape: handle duplicate label-texts in inventory

[Dominik Csapak <d.csapak@proxmox.com>]

find_media_by_label_text assumes that the label-texts are unique, but
currently this is not necessarily the case. To properly handle that,
change the signature to return a result, and in case there are duplicate
ones, return an error.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/drive.rs                | 69 +++++++++++++++++++--------
 src/api2/tape/media.rs                |  4 +-
 src/tape/changer/online_status_map.rs | 26 +++++-----
 src/tape/inventory.rs                 | 26 ++++++----
 4 files changed, 82 insertions(+), 43 deletions(-)

diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs
index 3a77ca6f..54d15db8 100644
--- a/src/api2/tape/drive.rs
+++ b/src/api2/tape/drive.rs
@@ -828,17 +828,27 @@ pub async fn inventory(drive: String) -> Result<Vec<LabelUuidMap>, Error> {
 
             let label_text = label_text.to_string();
 
-            if let Some(media_id) = inventory.find_media_by_label_text(&label_text) {
-                list.push(LabelUuidMap {
-                    label_text,
-                    uuid: Some(media_id.label.uuid.clone()),
-                });
-            } else {
-                list.push(LabelUuidMap {
-                    label_text,
-                    uuid: None,
-                });
-            }
+            match inventory.find_media_by_label_text(&label_text) {
+                Ok(Some(media_id)) => {
+                    list.push(LabelUuidMap {
+                        label_text,
+                        uuid: Some(media_id.label.uuid.clone()),
+                    });
+                }
+                Ok(None) => {
+                    list.push(LabelUuidMap {
+                        label_text,
+                        uuid: None,
+                    });
+                }
+                Err(err) => {
+                    log::warn!("error getting unique media label: {err}");
+                    list.push(LabelUuidMap {
+                        label_text,
+                        uuid: None,
+                    });
+                }
+            };
         }
 
         Ok(list)
@@ -916,11 +926,21 @@ pub fn update_inventory(
                 let label_text = label_text.to_string();
 
                 if !read_all_labels {
-                    if let Some(media_id) = inventory.find_media_by_label_text(&label_text) {
-                        if !catalog || MediaCatalog::exists(TAPE_STATUS_DIR, &media_id.label.uuid) {
-                            task_log!(worker, "media '{}' already inventoried", label_text);
+                    match inventory.find_media_by_label_text(&label_text) {
+                        Ok(Some(media_id)) => {
+                            if !catalog
+                                || MediaCatalog::exists(TAPE_STATUS_DIR, &media_id.label.uuid)
+                            {
+                                task_log!(worker, "media '{}' already inventoried", label_text);
+                                continue;
+                            }
+                        }
+                        Err(err) => {
+                            task_warn!(worker, "error getting media by unique label: {err}");
+                            // we can't be sure which uuid it is
                             continue;
                         }
+                        Ok(None) => {} // ok to inventorize
                     }
                 }
 
@@ -1079,13 +1099,20 @@ fn barcode_label_media_worker(
         }
 
         inventory.reload()?;
-        if inventory.find_media_by_label_text(&label_text).is_some() {
-            task_log!(
-                worker,
-                "media '{}' already inventoried (already labeled)",
-                label_text
-            );
-            continue;
+        match inventory.find_media_by_label_text(&label_text) {
+            Ok(Some(_)) => {
+                task_log!(
+                    worker,
+                    "media '{}' already inventoried (already labeled)",
+                    label_text
+                );
+                continue;
+            }
+            Err(err) => {
+                task_warn!(worker, "error getting media by unique label: {err}",);
+                continue;
+            }
+            Ok(None) => {} // ok to label
         }
 
         task_log!(worker, "checking/loading media '{}'", label_text);
diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs
index 3e8e1a9d..5031817c 100644
--- a/src/api2/tape/media.rs
+++ b/src/api2/tape/media.rs
@@ -303,7 +303,7 @@ pub fn move_tape(label_text: String, vault_name: Option<String>) -> Result<(), E
     let mut inventory = Inventory::load(TAPE_STATUS_DIR)?;
 
     let uuid = inventory
-        .find_media_by_label_text(&label_text)
+        .find_media_by_label_text(&label_text)?
         .ok_or_else(|| format_err!("no such media '{}'", label_text))?
         .label
         .uuid
@@ -339,7 +339,7 @@ pub fn destroy_media(label_text: String, force: Option<bool>) -> Result<(), Erro
     let mut inventory = Inventory::load(TAPE_STATUS_DIR)?;
 
     let media_id = inventory
-        .find_media_by_label_text(&label_text)
+        .find_media_by_label_text(&label_text)?
         .ok_or_else(|| format_err!("no such media '{}'", label_text))?;
 
     if !force {
diff --git a/src/tape/changer/online_status_map.rs b/src/tape/changer/online_status_map.rs
index 12a6220d..c3da0415 100644
--- a/src/tape/changer/online_status_map.rs
+++ b/src/tape/changer/online_status_map.rs
@@ -87,6 +87,16 @@ impl OnlineStatusMap {
     }
 }
 
+fn insert_into_online_set(inventory: &Inventory, label_text: &str, online_set: &mut HashSet<Uuid>) {
+    match inventory.find_media_by_label_text(&label_text) {
+        Ok(Some(media_id)) => {
+            online_set.insert(media_id.label.uuid.clone());
+        }
+        Ok(None) => {}
+        Err(err) => log::warn!("error getting media by unique label: {err}"),
+    }
+}
+
 /// Extract the list of online media from MtxStatus
 ///
 /// Returns a HashSet containing all found media Uuid. This only
@@ -96,9 +106,7 @@ pub fn mtx_status_to_online_set(status: &MtxStatus, inventory: &Inventory) -> Ha
 
     for drive_status in status.drives.iter() {
         if let ElementStatus::VolumeTag(ref label_text) = drive_status.status {
-            if let Some(media_id) = inventory.find_media_by_label_text(label_text) {
-                online_set.insert(media_id.label.uuid.clone());
-            }
+            insert_into_online_set(inventory, label_text, &mut online_set);
         }
     }
 
@@ -107,9 +115,7 @@ pub fn mtx_status_to_online_set(status: &MtxStatus, inventory: &Inventory) -> Ha
             continue;
         }
         if let ElementStatus::VolumeTag(ref label_text) = slot_info.status {
-            if let Some(media_id) = inventory.find_media_by_label_text(label_text) {
-                online_set.insert(media_id.label.uuid.clone());
-            }
+            insert_into_online_set(inventory, label_text, &mut online_set);
         }
     }
 
@@ -174,9 +180,7 @@ pub fn update_online_status<P: AsRef<Path>>(
 
         let mut online_set = HashSet::new();
         for label_text in media_list {
-            if let Some(media_id) = inventory.find_media_by_label_text(&label_text) {
-                online_set.insert(media_id.label.uuid.clone());
-            }
+            insert_into_online_set(&inventory, &label_text, &mut online_set);
         }
         map.update_online_status(&vtape.name, online_set)?;
     }
@@ -205,9 +209,7 @@ pub fn update_changer_online_status(
     let mut online_map = OnlineStatusMap::new(drive_config)?;
     let mut online_set = HashSet::new();
     for label_text in label_text_list.iter() {
-        if let Some(media_id) = inventory.find_media_by_label_text(label_text) {
-            online_set.insert(media_id.label.uuid.clone());
-        }
+        insert_into_online_set(inventory, label_text, &mut online_set)
     }
     online_map.update_online_status(changer_name, online_set)?;
     inventory.update_online_status(&online_map)?;
diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
index b84bd346..0b48c4a4 100644
--- a/src/tape/inventory.rs
+++ b/src/tape/inventory.rs
@@ -244,14 +244,24 @@ impl Inventory {
     }
 
     /// find media by label_text
-    pub fn find_media_by_label_text(&self, label_text: &str) -> Option<&MediaId> {
-        self.map.values().find_map(|entry| {
-            if entry.id.label.label_text == label_text {
-                Some(&entry.id)
-            } else {
-                None
-            }
-        })
+    pub fn find_media_by_label_text(&self, label_text: &str) -> Result<Option<&MediaId>, Error> {
+        let ids: Vec<_> = self
+            .map
+            .values()
+            .filter_map(|entry| {
+                if entry.id.label.label_text == label_text {
+                    Some(&entry.id)
+                } else {
+                    None
+                }
+            })
+            .collect();
+
+        match ids.len() {
+            0 => Ok(None),
+            1 => Ok(Some(ids[0])),
+            count => bail!("There are '{count}' tapes with the label '{label_text}'"),
+        }
     }
 
     /// Lookup media pool
-- 
2.30.2