From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 52881C0715 for ; Thu, 11 Jan 2024 11:40:39 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3C463F64C for ; Thu, 11 Jan 2024 11:40:39 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 11 Jan 2024 11:40:38 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E245647122 for ; Thu, 11 Jan 2024 11:40:37 +0100 (CET) From: Dominik Csapak To: pbs-devel@lists.proxmox.com Date: Thu, 11 Jan 2024 11:40:32 +0100 Message-Id: <20240111104036.3341854-2-d.csapak@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20240111104036.3341854-1-d.csapak@proxmox.com> References: <20240111104036.3341854-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.020 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pbs-devel] [PATCH proxmox-backup 1/5] tape: handle duplicate label-texts in inventory X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jan 2024 10:40:39 -0000 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 --- 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, 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) -> 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) -> 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) { + 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>( 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, 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