Re: [PATCH v1 proxmox-backup 3/3] datastore: introduce helper function to parse digest from S3ObjectKey

[Christian Ebner <c.ebner@proxmox.com>]

Very nice refactoring, makes the code much more robust and portable!

Again only one nit with which addressed:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

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 <r.obkircher@proxmox.com>
> ---
>   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::<str>(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<ProcessLockSharedGuard, Error> {
> 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"))
> +    );
> +}