[pbs-devel] [PATCH proxmox-backup-qemu 1/2] fix: never append archive name extensions twice, use type instead

[Markus Frank]

Thanks for the fix.

I have run a few qmrestore tasks with this patch applied.

pbs-restore now works as expected (with both .img.fidx and .img as filename extensions).

Tested-by: Markus Frank <m.frank at proxmox.com>

On  2025-01-28 16:09, Christian Ebner wrote:
> Commit 16298c5f ("update to current PBS master") introduced a helper
> method to generate the backup archive names from a device name.
> 
> However, the helper was also used incorrectly to generate archive
> names from strings already having the `.img.fidx` filename extension.
> As the extension is appended unconditionally, this leads to an
> archive name which will parse successfully, but not be the expected
> archive name to be found in the backup manifest.
> 
> Fix this by dropping the incorrect helper call-sites and adapt the
> function parameters to take a `BackupArchiveName` directly, reducing
> potential for future mishandling.
> 
> Fixes: Commit 16298c5f ("update to current PBS master")
> Reported-by: Markus Frank <m.frank at proxmox.com>
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
>   src/lib.rs     | 14 +++++++++++---
>   src/restore.rs | 10 ++++------
>   2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/lib.rs b/src/lib.rs
> index 0a1239b..ed94c9c 100644
> --- a/src/lib.rs
> +++ b/src/lib.rs
> @@ -8,7 +8,7 @@ use std::sync::{Arc, Condvar, Mutex};
>   
>   use proxmox_lang::try_block;
>   
> -use pbs_api_types::{Authid, BackupDir, BackupNamespace, BackupType, CryptMode};
> +use pbs_api_types::{Authid, BackupArchiveName, BackupDir, BackupNamespace, BackupType, CryptMode};
>   use pbs_client::BackupRepository;
>   
>   pub mod capi_types;
> @@ -979,6 +979,7 @@ pub extern "C" fn proxmox_restore_image(
>       let result: Result<_, Error> = try_block!({
>           let archive_name = tools::utf8_c_string(archive_name)?
>               .ok_or_else(|| format_err!("archive_name must not be NULL"))?;
> +        let archive_name: BackupArchiveName = archive_name.parse()?;
>   
>           let write_data_callback = move |offset: u64, data: &[u8]| {
>               callback(callback_data, offset, data.as_ptr(), data.len() as u64)
> @@ -988,7 +989,7 @@ pub extern "C" fn proxmox_restore_image(
>               move |offset: u64, len: u64| callback(callback_data, offset, std::ptr::null(), len);
>   
>           proxmox_async::runtime::block_on(restore_task.restore_image(
> -            archive_name,
> +            &archive_name,
>               write_data_callback,
>               write_zero_callback,
>               verbose,
> @@ -1051,9 +1052,16 @@ pub extern "C" fn proxmox_restore_open_image_async(
>   
>       param_not_null!(archive_name, callback_info);
>       let archive_name = unsafe { tools::utf8_c_string_lossy_non_null(archive_name) };
> +    let archive_name = match archive_name.parse() {
> +        Ok(archive_name) => archive_name,
> +        Err(err) => {
> +            callback_info.send_result(Err(err));
> +            return;
> +        }
> +    };
>   
>       restore_task.runtime().spawn(async move {
> -        let result = match restore_task.open_image(archive_name).await {
> +        let result = match restore_task.open_image(&archive_name).await {
>               Ok(res) => Ok(res as i32),
>               Err(err) => Err(err),
>           };
> diff --git a/src/restore.rs b/src/restore.rs
> index d3818e9..a879b99 100644
> --- a/src/restore.rs
> +++ b/src/restore.rs
> @@ -3,6 +3,7 @@ use std::sync::{Arc, Mutex};
>   
>   use anyhow::{bail, format_err, Error};
>   use once_cell::sync::OnceCell;
> +use pbs_api_types::BackupArchiveName;
>   use tokio::runtime::Runtime;
>   
>   use proxmox_async::runtime::get_runtime_with_builder;
> @@ -19,7 +20,6 @@ use pbs_tools::crypt_config::CryptConfig;
>   
>   use super::BackupSetup;
>   use crate::capi_types::DataPointer;
> -use crate::commands::archive_name;
>   use crate::registry::Registry;
>   use crate::shared_cache::get_shared_chunk_cache;
>   
> @@ -119,12 +119,11 @@ impl RestoreTask {
>   
>       pub async fn restore_image(
>           &self,
> -        archive_str: String,
> +        archive_name: &BackupArchiveName,
>           write_data_callback: impl Fn(u64, &[u8]) -> i32,
>           write_zero_callback: impl Fn(u64, u64) -> i32,
>           verbose: bool,
>       ) -> Result<(), Error> {
> -        let archive_name = archive_name(&archive_str)?;
>           if verbose {
>               eprintln!("download and verify backup index");
>           }
> @@ -218,8 +217,7 @@ impl RestoreTask {
>           Ok(info.archive_size)
>       }
>   
> -    pub async fn open_image(&self, archive_str: String) -> Result<u8, Error> {
> -        let archive_name = archive_name(&archive_str)?;
> +    pub async fn open_image(&self, archive_name: &BackupArchiveName) -> Result<u8, Error> {
>           let client = match self.client.get() {
>               Some(reader) => Arc::clone(reader),
>               None => bail!("not connected"),
> @@ -254,7 +252,7 @@ impl RestoreTask {
>   
>           let info = ImageAccessInfo {
>               archive_size,
> -            _archive_name: archive_str,
> +            _archive_name: archive_name.to_string(),
>               // useful to debug
>               reader,
>           };