From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 40A231FF13A for ; Wed, 22 Jul 2026 16:22:10 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EE319214D3; Wed, 22 Jul 2026 16:22:09 +0200 (CEST) Message-ID: <1296e2cd-113d-42f0-8508-65e5fb9caf7b@proxmox.com> Date: Wed, 22 Jul 2026 16:22:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [PATCH v1 proxmox-backup 3/3] datastore: introduce helper function to parse digest from S3ObjectKey To: Robert Obkircher , pbs-devel@lists.proxmox.com References: <20260713132338.170565-1-r.obkircher@proxmox.com> <20260713132338.170565-4-r.obkircher@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20260713132338.170565-4-r.obkircher@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784730096665 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.052 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_LOTSOFHASH 0.25 Emails with lots of hash-like gibberish RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UBHC72YT2GLFYP64WQHPJCC5UWDNB7BK X-Message-ID-Hash: UBHC72YT2GLFYP64WQHPJCC5UWDNB7BK X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Very nice refactoring, makes the code much more robust and portable! Again only one nit with which addressed: Reviewed-by: Christian Ebner On 7/13/26 3:24 PM, Robert Obkircher wrote: > Do not rely on the platform-specific `Path::file_name` because S3 > object keys are flat identifiers that always use `/` by convention. > It would, for example, be incorrect to ignore trailing slashes (or to > treat backslashes differently on Windows). > > Signed-off-by: Robert Obkircher > --- > pbs-datastore/src/datastore.rs | 28 ++++++------------------ > pbs-datastore/src/s3.rs | 39 +++++++++++++++++++++++++++++++--- > 2 files changed, 43 insertions(+), 24 deletions(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index bac5ce3b4..d106f5b52 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -48,7 +48,7 @@ use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter}; > use crate::fixed_index::{FixedIndexReader, FixedIndexWriter}; > use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive}; > use crate::index::IndexFile; > -use crate::s3::S3_CONTENT_PREFIX; > +use crate::s3::{S3_CONTENT_PREFIX, digest_from_object_key}; > use crate::task_tracking::{self, update_active_operations}; > use crate::{DataBlob, LocalDatastoreLruCache}; > > @@ -2675,36 +2675,22 @@ impl DataStore { > &self, > object_key: &S3ObjectKey, > ) -> Option<(PathBuf, [u8; 32], bool)> { > - // Check object is actually a chunk > - let path = Path::new::(object_key); > - // file_name() should always be Some, as objects will have a filename > - let file_name = path.file_name()?; > - let bytes = file_name.as_bytes(); > - let bad_chunk = if is_bad_chunk_suffix(&bytes[64..]) { > + let (file_name, digest, suffix) = digest_from_object_key(object_key)?; > + > + let bad_chunk = if is_bad_chunk_suffix(suffix.as_bytes()) { > true > - } else if bytes.len() == 64 { > + } else if suffix.is_empty() { > false > } else { > return None; // unexpected suffix > }; > - if !bytes.iter().take(64).all(u8::is_ascii_hexdigit) { > - return None; > - } > > - // Safe since contains valid ascii hexdigits only as checked above. > - let digest_str = file_name.to_string_lossy(); > - let hexdigit_prefix = unsafe { digest_str.get_unchecked(0..4) }; > let mut chunk_path = self.base_path(); > chunk_path.push(".chunks"); > - chunk_path.push(hexdigit_prefix); > + chunk_path.push(&file_name[..4]); > chunk_path.push(file_name); > > - let mut digest_bytes = [0u8; 32]; > - let digest = file_name.as_bytes(); > - // safe to unwrap as already checked above > - hex::decode_to_slice(&digest[..64], &mut digest_bytes).unwrap(); > - > - Some((chunk_path, digest_bytes, bad_chunk)) > + Some((chunk_path, digest, bad_chunk)) > } > > pub fn try_shared_chunk_store_lock(&self) -> Result { > diff --git a/pbs-datastore/src/s3.rs b/pbs-datastore/src/s3.rs > index 09affc2e5..c4209d60a 100644 > --- a/pbs-datastore/src/s3.rs > +++ b/pbs-datastore/src/s3.rs > @@ -1,6 +1,7 @@ > use std::path::{Path, PathBuf}; > > use anyhow::{Error, bail, format_err}; > +use hex::FromHex; > > use proxmox_s3_client::{DeleteObjectError, S3ObjectKey}; > > @@ -48,6 +49,16 @@ pub fn object_key_from_digest_with_suffix( > S3ObjectKey::try_from(object_key_string.as_str()) > } > > +/// Extract filename, digest, and suffix from the last part of an S3ObjectKey. > +pub fn digest_from_object_key(key: &S3ObjectKey) -> Option<(&str, [u8; 32], &str)> { nit: should be pub(crate) only > + let filename = key.rsplit('/').next()?; > + let (hex_digest, suffix) = filename.split_at_checked(64)?; > + > + let digest = FromHex::from_hex(hex_digest).ok()?; > + > + Some((filename, digest, suffix)) > +} > + > /// Log errors from delete objects api calls > pub(crate) fn log_s3_delete_objects_errors(errors: &[DeleteObjectError]) { > for error in errors { > @@ -96,7 +107,6 @@ fn test_object_key_from_path_incorrect_filename() { > > #[test] > fn test_object_key_from_digest() { > - use hex::FromHex; > let digest = > <[u8; 32]>::from_hex("bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8") > .unwrap(); > @@ -108,7 +118,6 @@ fn test_object_key_from_digest() { > > #[test] > fn test_object_key_from_digest_with_suffix() { > - use hex::FromHex; > let digest = > <[u8; 32]>::from_hex("bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8") > .unwrap(); > @@ -122,9 +131,33 @@ fn test_object_key_from_digest_with_suffix() { > > #[test] > fn test_object_key_from_digest_with_invalid_suffix() { > - use hex::FromHex; > let digest = > <[u8; 32]>::from_hex("bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8") > .unwrap(); > assert!(object_key_from_digest_with_suffix(&digest, "/.0.bad").is_err()); > } > + > +#[test] > +fn test_digest_from_object_key() { > + let hex = "bb9f8df61474d25e71fa00722318cd387396ca1736605e1248821cc0de3d3af8"; > + let digest = <[u8; 32]>::from_hex(hex).unwrap(); > + let k = |s: &str| S3ObjectKey::try_from(s).unwrap(); > + > + assert_eq!(digest_from_object_key(&k(&hex[1..])), None); > + let invalid_hex = hex.replace("b", "X"); > + assert_eq!(digest_from_object_key(&k(&invalid_hex)), None); > + > + assert_eq!(digest_from_object_key(&k(hex)), Some((hex, digest, ""))); > + > + let k1 = object_key_from_digest_with_suffix(&digest, "suffix1").unwrap(); > + assert_eq!( > + digest_from_object_key(&k1), > + Some((format!("{hex}suffix1").as_str(), digest, "suffix1")) > + ); > + > + let k2 = S3ObjectKey::try_from(format!(".chunks/bb9f/{hex}.0.bad").as_str()).unwrap(); > + assert_eq!( > + digest_from_object_key(&k2), > + Some((format!("{hex}.0.bad").as_str(), digest, ".0.bad")) > + ); > +}