[pbs-devel] [PATCH proxmox-backup 0/5] improve duplicate label-text handling

[pbs-devel] [PATCH proxmox-backup 0/5] improve duplicate label-text handling

[Dominik Csapak]

while most of the code assumes each label is unique in the inventory,
there is actually no safeguard against doing that.

this can lead to confusion about which tape is
inserted/online/destroyed/vaulted/etc.

so this series tries to improve the handling a bit by logging a warning
when encountering this, and also preventing new tapes being labeled with
an existing label

also add the 'destroy' media function to the ui, so that users
can get more easily out of this situation

Dominik Csapak (5):
  tape: handle duplicate label-texts in inventory
  api: tape: optinally accept uuid for destroy/move media
  api: tape: don't allow duplicate media label-texts
  ui: tape inventory: use uuid as id
  ui: tape: add remove media button

 src/api2/tape/drive.rs                | 79 +++++++++++++++++--------
 src/api2/tape/media.rs                | 84 ++++++++++++++++++++++-----
 src/tape/changer/online_status_map.rs | 26 +++++----
 src/tape/inventory.rs                 | 26 ++++++---
 www/Makefile                          |  1 +
 www/tape/TapeInventory.js             | 35 ++++++++++-
 www/tape/window/MediaRemoveWindow.js  | 66 +++++++++++++++++++++
 7 files changed, 257 insertions(+), 60 deletions(-)
 create mode 100644 www/tape/window/MediaRemoveWindow.js

-- 
2.30.2

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

[Dominik Csapak]

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

[pbs-devel] [PATCH proxmox-backup 2/5] api: tape: optinally accept uuid for destroy/move media

[Dominik Csapak]

so we can uniquely identify the tapes with duplicate labels.
The change is intended to be backwards compatible.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/media.rs | 84 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs
index 5031817c..07bed86a 100644
--- a/src/api2/tape/media.rs
+++ b/src/api2/tape/media.rs
@@ -3,7 +3,7 @@ use std::collections::HashSet;
 use anyhow::{bail, format_err, Error};
 
 use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
-use proxmox_schema::api;
+use proxmox_schema::{api, param_bail};
 use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
@@ -290,6 +290,11 @@ pub async fn list_media(
         properties: {
             "label-text": {
                 schema: MEDIA_LABEL_SCHEMA,
+                optional: true,
+            },
+            uuid: {
+                schema: MEDIA_UUID_SCHEMA,
+                optional: true,
             },
             "vault-name": {
                 schema: VAULT_NAME_SCHEMA,
@@ -299,15 +304,33 @@ pub async fn list_media(
     },
 )]
 /// Change Tape location to vault (if given), or offline.
-pub fn move_tape(label_text: String, vault_name: Option<String>) -> Result<(), Error> {
+pub fn move_tape(
+    label_text: Option<String>,
+    uuid: Option<Uuid>,
+    vault_name: Option<String>,
+) -> Result<(), Error> {
     let mut inventory = Inventory::load(TAPE_STATUS_DIR)?;
 
-    let uuid = inventory
-        .find_media_by_label_text(&label_text)?
-        .ok_or_else(|| format_err!("no such media '{}'", label_text))?
-        .label
-        .uuid
-        .clone();
+    let uuid = match (uuid, label_text) {
+        (Some(_), Some(_)) => {
+            param_bail!(
+                "format-text",
+                format_err!("A uuid is given, no label-text is expected.")
+            );
+        }
+        (None, None) => {
+            param_bail!(
+                "uuid",
+                format_err!("No label-text is given, a uuid is required.")
+            );
+        }
+        (Some(uuid), None) => uuid,
+        (None, Some(label_text)) => match inventory.find_media_by_label_text(&label_text) {
+            Ok(Some(media_id)) => media_id.label.uuid.clone(),
+            Ok(None) => bail!("no such media '{}'", label_text),
+            Err(err) => bail!("error getting media from unique label: {err}"),
+        },
+    };
 
     if let Some(vault_name) = vault_name {
         inventory.set_media_location_vault(&uuid, &vault_name)?;
@@ -323,6 +346,11 @@ pub fn move_tape(label_text: String, vault_name: Option<String>) -> Result<(), E
         properties: {
             "label-text": {
                 schema: MEDIA_LABEL_SCHEMA,
+                optional: true,
+            },
+            uuid: {
+                schema: MEDIA_UUID_SCHEMA,
+                optional: true,
             },
             force: {
                 description: "Force removal (even if media is used in a media set).",
@@ -333,22 +361,46 @@ pub fn move_tape(label_text: String, vault_name: Option<String>) -> Result<(), E
     },
 )]
 /// Destroy media (completely remove from database)
-pub fn destroy_media(label_text: String, force: Option<bool>) -> Result<(), Error> {
+pub fn destroy_media(
+    label_text: Option<String>,
+    uuid: Option<Uuid>,
+    force: Option<bool>,
+) -> Result<(), Error> {
     let force = force.unwrap_or(false);
 
     let mut inventory = Inventory::load(TAPE_STATUS_DIR)?;
 
-    let media_id = inventory
-        .find_media_by_label_text(&label_text)?
-        .ok_or_else(|| format_err!("no such media '{}'", label_text))?;
+    let (media_id, text) = match (uuid, label_text) {
+        (Some(_), Some(_)) => {
+            param_bail!(
+                "format-text",
+                format_err!("A uuid is given, no label-text is expected.")
+            );
+        }
+        (None, None) => {
+            param_bail!(
+                "uuid",
+                format_err!("No label-text is given, a uuid is required.")
+            );
+        }
+        (Some(uuid), None) => (
+            inventory
+                .lookup_media(&uuid)
+                .ok_or_else(|| format_err!("no such media '{}'", uuid))?,
+            uuid.to_string(),
+        ),
+        (None, Some(label_text)) => (
+            inventory
+                .find_media_by_label_text(&label_text)?
+                .ok_or_else(|| format_err!("no such media '{}'", label_text))?,
+            label_text,
+        ),
+    };
 
     if !force {
         if let Some(ref set) = media_id.media_set_label {
             if !set.unassigned() {
-                bail!(
-                    "media '{}' contains data (please use 'force' flag to remove.",
-                    label_text
-                );
+                bail!("media '{text}' contains data (please use 'force' flag to remove.");
             }
         }
     }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/5] api: tape: don't allow duplicate media label-texts

[Dominik Csapak]

quite a few parts of our code assumes that the label-text is unique in
the inventory, which leads to rather unexpected behaviour when having
more than one tape with the same label-text, e.g. a
`proxmox-tape media destroy <LABEL>`
destroys the first one in the config
(same with moving to vault, etc.)

since having multiple tapes with the same human readable name is always
confusing, simply disallow that here

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/drive.rs | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs
index 54d15db8..19c8e78a 100644
--- a/src/api2/tape/drive.rs
+++ b/src/api2/tape/drive.rs
@@ -539,6 +539,14 @@ fn write_media_label(
     label: MediaLabel,
     pool: Option<String>,
 ) -> Result<(), Error> {
+    let mut inventory = Inventory::new(TAPE_STATUS_DIR);
+    inventory.reload()?;
+    if inventory
+        .find_media_by_label_text(&label.label_text)?
+        .is_some()
+    {
+        bail!("Media with label '{}' already exists", label.label_text);
+    }
     drive.label_tape(&label)?;
     if let Some(ref pool) = pool {
         task_log!(
@@ -562,8 +570,6 @@ fn write_media_label(
 
     // Create the media catalog
     MediaCatalog::overwrite(TAPE_STATUS_DIR, &media_id, false)?;
-
-    let mut inventory = Inventory::new(TAPE_STATUS_DIR);
     inventory.store(media_id.clone(), false)?;
 
     drive.rewind()?;
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 4/5] ui: tape inventory: use uuid as id

[Dominik Csapak]

and add it as a hidden column. This now displays all tapes even if there
are some with identical label-texts.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/TapeInventory.js | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/www/tape/TapeInventory.js b/www/tape/TapeInventory.js
index 46dddc4a..dba36dac 100644
--- a/www/tape/TapeInventory.js
+++ b/www/tape/TapeInventory.js
@@ -17,7 +17,7 @@ Ext.define('pbs-model-tapes', {
 	'status',
 	'uuid',
     ],
-    idProperty: 'label-text',
+    idProperty: 'uuid',
     proxy: {
 	type: 'proxmox',
 	url: '/api2/json/tape/media/list',
@@ -293,5 +293,11 @@ Ext.define('PBS.TapeManagement.TapeInventory', {
 	    },
 	    flex: 1,
 	},
+	{
+	    text: gettext('UUID'),
+	    dataIndex: 'uuid',
+	    flex: 1,
+	    hidden: true,
+	},
     ],
 });
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 5/5] ui: tape: add remove media button

[Dominik Csapak]

this only removes media from the inventory, it does not touch the data

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
not super sure about the modal dialog, if we should expose 'force' at
all, and if we maybe should word it differently and/or add a hint
with a deeper explanation...

 www/Makefile                         |  1 +
 www/tape/TapeInventory.js            | 27 ++++++++++++
 www/tape/window/MediaRemoveWindow.js | 66 ++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 www/tape/window/MediaRemoveWindow.js

diff --git a/www/Makefile b/www/Makefile
index be7e27ab..c2755ac8 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -21,6 +21,7 @@ TAPE_UI_FILES=						\
 	tape/window/Erase.js				\
 	tape/window/EncryptionEdit.js			\
 	tape/window/LabelMedia.js			\
+	tape/window/MediaRemoveWindow.js		\
 	tape/window/PoolEdit.js				\
 	tape/window/TapeBackup.js			\
 	tape/window/TapeBackupJob.js			\
diff --git a/www/tape/TapeInventory.js b/www/tape/TapeInventory.js
index dba36dac..3039a95a 100644
--- a/www/tape/TapeInventory.js
+++ b/www/tape/TapeInventory.js
@@ -60,6 +60,27 @@ Ext.define('PBS.TapeManagement.TapeInventory', {
 	    }).show();
 	},
 
+	remove: function() {
+	    let me = this;
+	    let view = me.getView();
+	    let selection = view.getSelection();
+	    if (!selection || selection.length < 1) {
+		return;
+	    }
+	    let uuid = selection[0].data.uuid;
+	    let label = selection[0].data['label-text'];
+	    Ext.create('PBS.TapeManagement.MediaRemoveWindow', {
+		uuid,
+		label,
+		autoShow: true,
+		listeners: {
+		    destroy: function() {
+			me.reload();
+		    },
+		},
+	    });
+	},
+
 	moveToVault: function() {
 	    let me = this;
 	    let view = me.getView();
@@ -206,6 +227,12 @@ Ext.define('PBS.TapeManagement.TapeInventory', {
 	    disabled: true,
 	    handler: 'format',
 	},
+	{
+	    xtype: 'proxmoxButton',
+	    text: gettext('Remove'),
+	    disabled: true,
+	    handler: 'remove',
+	},
     ],
 
     features: [
diff --git a/www/tape/window/MediaRemoveWindow.js b/www/tape/window/MediaRemoveWindow.js
new file mode 100644
index 00000000..0eb3d6be
--- /dev/null
+++ b/www/tape/window/MediaRemoveWindow.js
@@ -0,0 +1,66 @@
+Ext.define('PBS.TapeManagement.MediaRemoveWindow', {
+    extend: 'Proxmox.window.Edit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    uuid: undefined,
+    label: undefined,
+
+    cbindData: function(config) {
+	let me = this;
+	return {
+	    uuid: me.uuid,
+	    warning: Ext.String.format(gettext("Are you sure you want to remove tape '{0}' ?"), me.label),
+	};
+    },
+
+    title: gettext('Remove Media'),
+    url: `/api2/extjs/tape/media/destroy`,
+
+    layout: 'hbox',
+    width: 400,
+    method: 'GET',
+    isCreate: true,
+    submitText: gettext('Ok'),
+    items: [
+	{
+	    xtype: 'container',
+	    padding: 0,
+	    layout: {
+		type: 'hbox',
+		align: 'stretch',
+	    },
+	    items: [
+		{
+		    xtype: 'component',
+		    cls: [Ext.baseCSSPrefix + 'message-box-icon',
+			Ext.baseCSSPrefix + 'message-box-warning',
+			Ext.baseCSSPrefix + 'dlg-icon'],
+		},
+		{
+		    xtype: 'container',
+		    flex: 1,
+		    items: [
+			{
+			    xtype: 'displayfield',
+			    cbind: {
+				value: '{warning}',
+			    },
+			},
+			{
+			    xtype: 'hidden',
+			    name: 'uuid',
+			    cbind: {
+				value: '{uuid}',
+			    },
+			},
+			{
+			    xtype: 'proxmoxcheckbox',
+			    fieldLabel: gettext('Force'),
+			    name: 'force',
+			},
+		    ],
+		},
+	    ],
+	},
+    ],
+});
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 0/5] improve duplicate label-text handling

[Dominik Csapak]

sorry forgot the 'tape: ' tag in the cover letter

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

[Dietmar Maurer]

I am confused about insert_into_online_set(). We now do not add
the media to the MtxStatus, although the media is detected, only because there is another tape with the same label?

That is far more confusing than before, isnt it?
 
> +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
>

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

[Dominik Csapak]

On 1/12/24 08:29, Dietmar Maurer wrote:
> I am confused about insert_into_online_set(). We now do not add
> the media to the MtxStatus, although the media is detected, only because there is another tape with the same label?
> 
> That is far more confusing than before, isnt it?

but don't actually know which of the one in the inventory it is?
we only have the label and simple chose the first before, but that might be the wrong one...

>   
>> +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
>>

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

[Dietmar Maurer]

> > That is far more confusing than before, isnt it?
> 
> but don't actually know which of the one in the inventory it is?
> we only have the label and simple chose the first before, but that might be the wrong one...

I see the problem. Still, this is very confusing...

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

[Dominik Csapak]

On 1/12/24 09:01, Dietmar Maurer wrote:
>>> That is far more confusing than before, isnt it?
>>
>> but don't actually know which of the one in the inventory it is?
>> we only have the label and simple chose the first before, but that might be the wrong one...
> 
> I see the problem. Still, this is very confusing...

exactly, this is why the later patches prevent duplicate labels
and we now put out a warning into the syslog every time we encounter this...

[pbs-devel] applied: [PATCH proxmox-backup 0/5] improve duplicate label-text handling

[Dietmar Maurer]

applied

On 1/11/24 11:40, Dominik Csapak wrote:
> while most of the code assumes each label is unique in the inventory,
> there is actually no safeguard against doing that.
>
> this can lead to confusion about which tape is
> inserted/online/destroyed/vaulted/etc.
>
> so this series tries to improve the handling a bit by logging a warning
> when encountering this, and also preventing new tapes being labeled with
> an existing label
>
> also add the 'destroy' media function to the ui, so that users
> can get more easily out of this situation
>
> Dominik Csapak (5):
>    tape: handle duplicate label-texts in inventory
>    api: tape: optinally accept uuid for destroy/move media
>    api: tape: don't allow duplicate media label-texts
>    ui: tape inventory: use uuid as id
>    ui: tape: add remove media button
>
>   src/api2/tape/drive.rs                | 79 +++++++++++++++++--------
>   src/api2/tape/media.rs                | 84 ++++++++++++++++++++++-----
>   src/tape/changer/online_status_map.rs | 26 +++++----
>   src/tape/inventory.rs                 | 26 ++++++---
>   www/Makefile                          |  1 +
>   www/tape/TapeInventory.js             | 35 ++++++++++-
>   www/tape/window/MediaRemoveWindow.js  | 66 +++++++++++++++++++++
>   7 files changed, 257 insertions(+), 60 deletions(-)
>   create mode 100644 www/tape/window/MediaRemoveWindow.js
>