[PATCH v5 proxmox-backup 00/16] fix: #3847 pipe from STDIN to proxmox-backup-client

[PATCH v5 proxmox-backup 00/16] fix: #3847 pipe from STDIN to proxmox-backup-client

[Robert Obkircher]

Add support for commands like:
    ssh host cmd | proxmox-backup-client backup data.img:/dev/stdin
    proxmox-backup-client backup a.img:<(mysqldump) b.img:<(pgdump)

Changes since v4:
- reordered the ChunkStore removal commit and adjusted dynamic writer
- moved TempTestDir into separate file, use it for test_chunk_store1
- improved commit messages and comments
- changed u32 param to u64
- removed unnecessary casts reported by clippy
- added two commits at the end which restrict chunk_size to u32
  (I'd fixup commit 12/16 if we actually want to keep those changes)

An open question is if we should maybe fsync the data before renaming.
Or is that too much overhead for slightly better crash consistency?


Robert Obkircher (16):
  datastore: remove Arc<ChunkStore> from FixedIndexWriter
  datastore: remove Arc<ChunkStore> from DynamicIndexWriter
  datastore: add TempTestDir that is automatically deleted on drop
  datastore: use temporary directory for chunk store test
  datastore: support writing fidx files of unknown size
  datastore: test FixedIndexWriter
  api: backup: make fixed index file size optional
  api: verify fixed index writer size on close
  fix #3847: client: support fifo pipe inputs for images
  client: treat minus sign as stdin
  datastore: combine public FixedIndexWriter methods into add_chunk.
  datastore: use u64 instead of usize for fidx writer content size
  datastore: compute fidx file size with overflow checks
  datastore: support writing fidx files on systems with larger page size
  datastore: FixedIndexWriter: switch public chunk_size to u32
  datastore: FixedIndexWriter: switch internal chunk_size to u32

 pbs-client/src/backup_writer.rs    |  38 ++-
 pbs-datastore/src/chunk_store.rs   |   7 +-
 pbs-datastore/src/datastore.rs     |  19 +-
 pbs-datastore/src/dynamic_index.rs |   5 +-
 pbs-datastore/src/fixed_index.rs   | 435 +++++++++++++++++++++++++----
 pbs-datastore/src/lib.rs           |   3 +
 pbs-datastore/src/temp_test_dir.rs |  33 +++
 proxmox-backup-client/src/main.rs  |  37 ++-
 src/api2/backup/environment.rs     |  33 ++-
 src/api2/backup/mod.rs             |   6 +-
 src/server/push.rs                 |  11 +-
 11 files changed, 499 insertions(+), 128 deletions(-)
 create mode 100644 pbs-datastore/src/temp_test_dir.rs

-- 
2.47.3

[PATCH v5 proxmox-backup 01/16] datastore: remove Arc<ChunkStore> from FixedIndexWriter

[Robert Obkircher]

The ChunkStore reference can be safely removed, because it was only
needed for the base directory. As discussed previously in the context
of the DynamicIndexWriter [1], it is the callers responsibility to
ensure that the writer doesn't outlive any locks.

This simplifies testing the writer, which would otherwise require a
workaround to avoid creating 65k directories.

[1] https://lore.proxmox.com/pbs-devel/1761822027.sm41v71zmk.astroid@yuna.none/

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   | 10 ++--------
 pbs-datastore/src/fixed_index.rs |  7 ++-----
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 7ad3d917..1c631817 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -698,14 +698,8 @@ impl DataStore {
         size: usize,
         chunk_size: usize,
     ) -> Result<FixedIndexWriter, Error> {
-        let index = FixedIndexWriter::create(
-            self.inner.chunk_store.clone(),
-            filename.as_ref(),
-            size,
-            chunk_size,
-        )?;
-
-        Ok(index)
+        let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
+        FixedIndexWriter::create(full_path, size, chunk_size)
     }
 
     pub fn open_fixed_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 6c3be2d4..becbe3e3 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -4,14 +4,12 @@ use std::io::{Seek, SeekFrom};
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::ptr::NonNull;
-use std::sync::Arc;
 
 use anyhow::{bail, format_err, Error};
 
 use proxmox_io::ReadExt;
 use proxmox_uuid::Uuid;
 
-use crate::chunk_store::ChunkStore;
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
 
@@ -240,12 +238,11 @@ impl FixedIndexWriter {
     #[allow(clippy::cast_ptr_alignment)]
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
-        store: Arc<ChunkStore>,
-        path: &Path,
+        full_path: impl Into<PathBuf>,
         size: usize,
         chunk_size: usize,
     ) -> Result<Self, Error> {
-        let full_path = store.relative_path(path);
+        let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
         tmp_path.set_extension("tmp_fidx");
 
-- 
2.47.3

[PATCH v5 proxmox-backup 02/16] datastore: remove Arc<ChunkStore> from DynamicIndexWriter

[Robert Obkircher]

Remain consistent with the FixedIndexWriter and make it easier to add
tests in the future.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs     | 5 ++---
 pbs-datastore/src/dynamic_index.rs | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 1c631817..b023b0d3 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -718,9 +718,8 @@ impl DataStore {
         &self,
         filename: P,
     ) -> Result<DynamicIndexWriter, Error> {
-        let index = DynamicIndexWriter::create(self.inner.chunk_store.clone(), filename.as_ref())?;
-
-        Ok(index)
+        let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
+        DynamicIndexWriter::create(full_path)
     }
 
     pub fn open_dynamic_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index ad49cdf3..e2fe09a4 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -16,7 +16,6 @@ use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 
 use pbs_tools::lru_cache::LruCache;
 
-use crate::chunk_store::ChunkStore;
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
 use crate::read_chunk::ReadChunk;
@@ -289,8 +288,8 @@ impl Drop for DynamicIndexWriter {
 
 impl DynamicIndexWriter {
     // Requires obtaining a shared chunk store lock beforehand
-    pub fn create(store: Arc<ChunkStore>, path: &Path) -> Result<Self, Error> {
-        let full_path = store.relative_path(path);
+    pub fn create(full_path: impl Into<PathBuf>) -> Result<Self, Error> {
+        let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
         tmp_path.set_extension("tmp_didx");
 
-- 
2.47.3

[PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Robert Obkircher]

This will be used to test the FixedIndexWriter and chunk store
creation.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/lib.rs           |  4 ++++
 pbs-datastore/src/temp_test_dir.rs | 33 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 pbs-datastore/src/temp_test_dir.rs

diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 1f7c54ae..b48ec93c 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -233,3 +233,7 @@ pub use local_chunk_reader::LocalChunkReader;
 
 mod local_datastore_lru_cache;
 pub use local_datastore_lru_cache::LocalDatastoreLruCache;
+
+#[cfg(test)]
+mod temp_test_dir;
+
diff --git a/pbs-datastore/src/temp_test_dir.rs b/pbs-datastore/src/temp_test_dir.rs
new file mode 100644
index 00000000..13e16c91
--- /dev/null
+++ b/pbs-datastore/src/temp_test_dir.rs
@@ -0,0 +1,33 @@
+use std::convert::From;
+use std::env;
+use std::path::{Path, PathBuf};
+
+/// A temporary directory for tests that is removed on drop.
+/// Deletion is best-effort and errors are silently ignored.
+pub struct TempTestDir(PathBuf);
+
+impl TempTestDir {
+    /// Create an empty directory.
+    pub fn new() -> Self {
+        Self(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
+    }
+}
+
+impl std::ops::Deref for TempTestDir {
+    type Target = Path;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl Drop for TempTestDir {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.0);
+    }
+}
+
+impl<'a> From<&'a TempTestDir> for PathBuf {
+    fn from(temp: &'a TempTestDir) -> Self {
+        temp.0.clone()
+    }
+}
-- 
2.47.3

[PATCH v5 proxmox-backup 04/16] datastore: use temporary directory for chunk store test

[Robert Obkircher]

Use the systems tmp directory instead of the package root, because the
latter might not always be writeable and because the created files
weren't even ignored by git.

This reduced the tests runtime from 3.2 to 0.6 seconds.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index bd1dc353..a2763890 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -980,10 +980,7 @@ impl ChunkExt {
 
 #[test]
 fn test_chunk_store1() {
-    let mut path = std::fs::canonicalize(".").unwrap(); // we need absolute path
-    path.push(".testdir");
-
-    if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
+    let path = crate::temp_test_dir::TempTestDir::new();
 
     let chunk_store = ChunkStore::open("test", &path, DatastoreFSyncLevel::None);
     assert!(chunk_store.is_err());
@@ -1007,6 +1004,4 @@ fn test_chunk_store1() {
     let chunk_store =
         ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
     assert!(chunk_store.is_err());
-
-    if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
 }
-- 
2.47.3

[PATCH v5 proxmox-backup 05/16] datastore: support writing fidx files of unknown size

[Robert Obkircher]

Use mremap and ftruncate to support growable FixedIndexWriters. Grow
exponentially from a small initial index size and truncate excessive
capacity after encountering a non-full block or on close.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   |   2 +-
 pbs-datastore/src/fixed_index.rs | 127 ++++++++++++++++++++++++++++++-
 src/api2/backup/mod.rs           |   4 +-
 3 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index b023b0d3..694a5dd0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -695,7 +695,7 @@ impl DataStore {
     pub fn create_fixed_writer<P: AsRef<Path>>(
         &self,
         filename: P,
-        size: usize,
+        size: Option<usize>,
         chunk_size: usize,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index becbe3e3..bcb0cf94 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -217,9 +217,12 @@ pub struct FixedIndexWriter {
     chunk_size: usize,
     size: usize,
     index_length: usize,
+    index_capacity: usize,
     index: *mut u8,
     pub uuid: [u8; 16],
     pub ctime: i64,
+    growable_size: bool,
+    write_size_on_close: bool,
 }
 
 // `index` is mmap()ed which cannot be thread-local so should be sendable
@@ -235,11 +238,21 @@ impl Drop for FixedIndexWriter {
 }
 
 impl FixedIndexWriter {
+    /// The initial capacity, if the total size is unknown.
+    ///
+    /// This capacity takes up the same amount of space as the header
+    /// and can refer to 128 Blocks * 4 MiB/Block = 512 MiB of content.
+    ///
+    /// On systems with 4 KiB page size this value ensures that the
+    /// mapped length is a multiple of the page size, but this is not
+    /// strictly necessary.
+    const INITIAL_CAPACITY: usize = 4096 / 32;
+
     #[allow(clippy::cast_ptr_alignment)]
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
         full_path: impl Into<PathBuf>,
-        size: usize,
+        known_size: Option<usize>,
         chunk_size: usize,
     ) -> Result<Self, Error> {
         let full_path = full_path.into();
@@ -261,6 +274,7 @@ impl FixedIndexWriter {
         }
 
         let ctime = proxmox_time::epoch_i64();
+        let size = known_size.unwrap_or(0);
 
         let uuid = Uuid::generate();
 
@@ -277,8 +291,12 @@ impl FixedIndexWriter {
 
         file.write_all(&buffer)?;
 
-        let index_length = size.div_ceil(chunk_size);
-        let index_size = index_length * 32;
+        let (index_length, index_capacity) = known_size
+            .map(|s| s.div_ceil(chunk_size))
+            .map(|len| (len, len))
+            .unwrap_or((0, Self::INITIAL_CAPACITY));
+
+        let index_size = index_capacity * 32;
         nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
 
         let data = unsafe {
@@ -302,12 +320,90 @@ impl FixedIndexWriter {
             chunk_size,
             size,
             index_length,
+            index_capacity,
             index: data,
             ctime,
             uuid: *uuid.as_bytes(),
+            growable_size: known_size.is_none(),
+            write_size_on_close: known_size.is_none(),
+        })
+    }
+
+    /// If this returns an error, the sizes may be out of sync,
+    /// which is especially bad if the capacity was reduced.
+    fn set_index_capacity(&mut self, new_capacity: usize) -> Result<(), Error> {
+        if new_capacity == self.index_capacity {
+            return Ok(());
+        }
+        let old_index_size = self.index_capacity * 32;
+        let new_index_size = new_capacity * 32;
+        let new_file_size = (size_of::<FixedIndexHeader>() + new_index_size) as i64;
+
+        let index_addr = NonNull::new(self.index as *mut std::ffi::c_void).ok_or_else(|| {
+            format_err!("Can't resize FixedIndexWriter index because the index pointer is null.")
+        })?;
+
+        nix::unistd::ftruncate(&self.file, new_file_size)?;
+
+        let new_index = unsafe {
+            nix::sys::mman::mremap(
+                index_addr,
+                old_index_size,
+                new_index_size,
+                nix::sys::mman::MRemapFlags::MREMAP_MAYMOVE,
+                None,
+            )
+        }?;
+
+        self.index = new_index.as_ptr().cast::<u8>();
+        self.index_capacity = new_capacity;
+        Ok(())
+    }
+
+    /// Unmapping ensures future add and close operations fail.
+    fn set_index_capacity_or_unmap(&mut self, new_capacity: usize) -> Result<(), Error> {
+        self.set_index_capacity(new_capacity).map_err(|e| {
+            let unmap_result = self.unmap();
+            let message = format!(
+                "failed to resize index capacity from {} to {new_capacity} with backing file: {:?}",
+                self.index_capacity, self.tmp_filename
+            );
+            assert!(self.index.is_null(), "{message} {unmap_result:?}");
+            e.context(message)
         })
     }
 
+    /// Increase the content size to be at least `requested_size` and
+    /// ensure there is enough capacity.
+    ///
+    /// Only writers that were created without a known size can grow.
+    /// The size also becomes fixed as soon as it is no longer divisible
+    /// by the block size, to ensure that only the last block can be
+    /// smaller.
+    pub fn grow_to_size(&mut self, requested_size: usize) -> Result<(), Error> {
+        if self.size < requested_size {
+            if !self.growable_size {
+                bail!("refusing to resize from {} to {requested_size}", self.size);
+            }
+            let new_len = requested_size.div_ceil(self.chunk_size);
+            if new_len * self.chunk_size != requested_size {
+                // not a full chunk, so this must be the last one
+                self.growable_size = false;
+                self.set_index_capacity_or_unmap(new_len)?;
+            } else if new_len > self.index_capacity {
+                let new_capacity = new_len
+                    .checked_next_power_of_two()
+                    .ok_or_else(|| format_err!("capacity overflow"))?;
+                self.set_index_capacity_or_unmap(new_capacity)?;
+            }
+            assert!(new_len <= self.index_capacity);
+            self.index_length = new_len;
+            self.size = requested_size;
+        }
+        Ok(())
+    }
+
+    /// The current length of the index.
     pub fn index_length(&self) -> usize {
         self.index_length
     }
@@ -317,7 +413,7 @@ impl FixedIndexWriter {
             return Ok(());
         };
 
-        let index_size = self.index_length * 32;
+        let index_size = self.index_capacity * 32;
 
         if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
             bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
@@ -339,9 +435,24 @@ impl FixedIndexWriter {
 
         self.unmap()?;
 
+        if self.index_length == 0 {
+            bail!("refusing to close empty fidx file {:?}", self.tmp_filename);
+        } else if self.index_length < self.index_capacity {
+            let file_size = size_of::<FixedIndexHeader>() + index_size;
+            nix::unistd::ftruncate(&self.file, file_size as i64)?;
+            self.index_capacity = self.index_length;
+        }
+
         let csum_offset = std::mem::offset_of!(FixedIndexHeader, index_csum);
         self.file.seek(SeekFrom::Start(csum_offset as u64))?;
         self.file.write_all(&index_csum)?;
+
+        if self.write_size_on_close {
+            let size_offset = std::mem::offset_of!(FixedIndexHeader, size);
+            self.file.seek(SeekFrom::Start(size_offset as u64))?;
+            self.file.write_all(&(self.size as u64).to_le_bytes())?;
+        }
+
         self.file.flush()?;
 
         if let Err(err) = std::fs::rename(&self.tmp_filename, &self.filename) {
@@ -404,6 +515,14 @@ impl FixedIndexWriter {
     }
 
     pub fn clone_data_from(&mut self, reader: &FixedIndexReader) -> Result<(), Error> {
+        if self.growable_size {
+            bail!("reusing the index is only supported with known input size");
+        }
+
+        if self.chunk_size != reader.chunk_size {
+            bail!("can't reuse file with different chunk size");
+        }
+
         if self.index_length != reader.index_count() {
             bail!("clone_data_from failed - index sizes not equal");
         }
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 3e6b7a95..c625a2dc 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -528,7 +528,9 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
+    let mut writer = env
+        .datastore
+        .create_fixed_writer(&path, Some(size), chunk_size)?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
-- 
2.47.3

[PATCH v5 proxmox-backup 06/16] datastore: test FixedIndexWriter

[Robert Obkircher]

Write fixed and dynamically sized fidx files to a temporary directory.
Compare the resulting files directly (ignoring uuid and ctime bytes)
and also read them back using the reader.

The chunk hashes are just dummy values that don't actually exist in a
chunk store.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 170 +++++++++++++++++++++++++++++++
 pbs-datastore/src/lib.rs         |   1 -
 2 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index bcb0cf94..0f87bf15 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -534,3 +534,173 @@ impl FixedIndexWriter {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::fs;
+
+    use super::*;
+    use crate::temp_test_dir::TempTestDir;
+
+    const CS: usize = 4096;
+
+    #[test]
+    fn test_empty() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_empty");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        assert!(w.add_digest(0, &[1u8; 32]).is_err(), "out of bounds");
+
+        assert_eq!(0, w.size);
+        assert_eq!(0, w.index_length(), "returns length, not capacity");
+        assert_eq!(FixedIndexWriter::INITIAL_CAPACITY, w.index_capacity);
+
+        assert!(w.close().is_err(), "should refuse to create empty file");
+
+        drop(w);
+        assert!(!fs::exists(path).unwrap());
+    }
+
+    #[test]
+    fn test_single_partial_chunk() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_single_partial_chunk");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        let size = CS - 1;
+        let expected = test_data(size);
+        w.grow_to_size(size).unwrap();
+        expected[0].add_to(&mut w);
+
+        w.close().unwrap();
+        drop(w);
+
+        check_with_reader(&path, size, &expected);
+        compare_to_known_size_writer(&path, size, &expected);
+    }
+
+    #[test]
+    fn test_grow_to_multiples_of_chunk_size() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_grow_to_multiples_of_chunk_size");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        let initial = FixedIndexWriter::INITIAL_CAPACITY;
+        let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
+        let expected = test_data(steps.last().unwrap() * CS);
+
+        let mut begin = 0;
+        for chunk_count in steps {
+            let last = &expected[chunk_count - 1];
+            w.grow_to_size(last.end).unwrap();
+            assert_eq!(last.index + 1, w.index_length());
+            assert!(w.add_digest(last.index + 1, &[1u8; 32]).is_err());
+
+            for c in expected[begin..chunk_count].iter().rev() {
+                c.add_to(&mut w);
+            }
+            begin = chunk_count;
+        }
+        w.close().unwrap();
+        drop(w);
+
+        let size = expected.len() * CS;
+        check_with_reader(&path, size, &expected);
+        compare_to_known_size_writer(&path, size, &expected);
+    }
+
+    #[test]
+    fn test_grow_to_misaligned_size() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_grow_to_misaligned_size");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        let size = (FixedIndexWriter::INITIAL_CAPACITY + 42) * CS - 1; // last is not full
+        let expected = test_data(size);
+
+        w.grow_to_size(size).unwrap();
+        assert!(w.grow_to_size(size + 1).is_err(), "size must be fixed now");
+        assert_eq!(expected.len(), w.index_length());
+        assert!(w.add_digest(expected.len(), &[1u8; 32]).is_err());
+
+        for c in expected.iter().rev() {
+            c.add_to(&mut w);
+        }
+
+        w.close().unwrap();
+        drop(w);
+
+        check_with_reader(&path, size, &expected);
+        compare_to_known_size_writer(&path, size, &expected);
+    }
+
+    struct TestChunk {
+        digest: [u8; 32],
+        index: usize,
+        size: usize,
+        end: usize,
+    }
+
+    impl TestChunk {
+        fn add_to(&self, w: &mut FixedIndexWriter) {
+            assert_eq!(
+                self.index,
+                w.check_chunk_alignment(self.end, self.size).unwrap()
+            );
+            w.add_digest(self.index, &self.digest).unwrap();
+        }
+    }
+
+    fn test_data(size: usize) -> Vec<TestChunk> {
+        (0..size.div_ceil(CS))
+            .map(|index| {
+                let mut digest = [0u8; 32];
+                let i = &(index as u64).to_le_bytes();
+                for c in digest.chunks_mut(i.len()) {
+                    c.copy_from_slice(i);
+                }
+                let size = if ((index + 1) * CS) <= size {
+                    CS
+                } else {
+                    size % CS
+                };
+                TestChunk {
+                    digest,
+                    index,
+                    size,
+                    end: index * CS + size,
+                }
+            })
+            .collect()
+    }
+
+    fn check_with_reader(path: &Path, size: usize, chunks: &[TestChunk]) {
+        let reader = FixedIndexReader::open(path).unwrap();
+        assert_eq!(size as u64, reader.index_bytes());
+        assert_eq!(chunks.len(), reader.index_count());
+        for c in chunks {
+            assert_eq!(&c.digest, reader.index_digest(c.index).unwrap());
+        }
+    }
+
+    fn compare_to_known_size_writer(file: &Path, size: usize, chunks: &[TestChunk]) {
+        let mut path = file.to_path_buf();
+        path.set_extension("reference");
+        let mut w = FixedIndexWriter::create(&path, Some(size), CS).unwrap();
+        for c in chunks {
+            c.add_to(&mut w);
+        }
+        w.close().unwrap();
+        drop(w);
+
+        let mut reference = fs::read(file).unwrap();
+        let mut tested = fs::read(path).unwrap();
+
+        // ignore uuid and ctime
+        reference[8..32].fill(0);
+        tested[8..32].fill(0);
+
+        assert_eq!(reference, tested);
+    }
+}
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index b48ec93c..23a17d96 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -236,4 +236,3 @@ pub use local_datastore_lru_cache::LocalDatastoreLruCache;
 
 #[cfg(test)]
 mod temp_test_dir;
-
-- 
2.47.3

[PATCH v5 proxmox-backup 07/16] api: backup: make fixed index file size optional

[Robert Obkircher]

Pass the optional size to the FixedIndexWriter to make it grow as
necessary.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/api2/backup/environment.rs | 21 ++++++++++++---------
 src/api2/backup/mod.rs         |  8 +++-----
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index bd9c5211..90a84ed2 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -67,7 +67,7 @@ struct DynamicWriterState {
 struct FixedWriterState {
     name: String,
     index: FixedIndexWriter,
-    size: usize,
+    size: Option<usize>,
     chunk_size: u32,
     chunk_count: u64,
     small_chunk_count: usize, // allow 0..1 small chunks (last chunk may be smaller)
@@ -349,7 +349,7 @@ impl BackupEnvironment {
         &self,
         index: FixedIndexWriter,
         name: String,
-        size: usize,
+        size: Option<usize>,
         chunk_size: u32,
         incremental: bool,
     ) -> Result<usize, Error> {
@@ -443,6 +443,7 @@ impl BackupEnvironment {
         }
 
         let end = (offset as usize) + (size as usize);
+        data.index.grow_to_size(end)?;
         let idx = data.index.check_chunk_alignment(end, size as usize)?;
 
         data.chunk_count += 1;
@@ -624,13 +625,15 @@ impl BackupEnvironment {
                 );
             }
 
-            if size != (data.size as u64) {
-                bail!(
-                    "fixed writer '{}' close failed - unexpected file size ({} != {})",
-                    data.name,
-                    data.size,
-                    size
-                );
+            if let Some(known_size) = data.size {
+                if size != known_size as u64 {
+                    bail!(
+                        "fixed writer '{}' close failed - unexpected file size ({} != {})",
+                        data.name,
+                        known_size,
+                        size,
+                    );
+                }
             }
         }
 
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index c625a2dc..c2822c18 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -456,7 +456,7 @@ pub const API_METHOD_CREATE_FIXED_INDEX: ApiMethod = ApiMethod::new(
             ("archive-name", false, &BACKUP_ARCHIVE_NAME_SCHEMA),
             (
                 "size",
-                false,
+                true,
                 &IntegerSchema::new("File size.").minimum(1).schema()
             ),
             (
@@ -480,7 +480,7 @@ fn create_fixed_index(
     let env: &BackupEnvironment = rpcenv.as_ref();
 
     let name = required_string_param(&param, "archive-name")?.to_owned();
-    let size = required_integer_param(&param, "size")? as usize;
+    let size = param["size"].as_u64().map(usize::try_from).transpose()?;
     let reuse_csum = param["reuse-csum"].as_str();
 
     let archive_name = name.clone();
@@ -528,9 +528,7 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env
-        .datastore
-        .create_fixed_writer(&path, Some(size), chunk_size)?;
+    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
-- 
2.47.3

[PATCH v5 proxmox-backup 08/16] api: verify fixed index writer size on close

[Robert Obkircher]

Compare it to the value submitted by the client to check that the
resizing worked as expected.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 5 +++++
 src/api2/backup/environment.rs   | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 0f87bf15..c4fe899d 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -408,6 +408,11 @@ impl FixedIndexWriter {
         self.index_length
     }
 
+    /// The current total size of the referenced content.
+    pub fn size(&self) -> u64 {
+        self.size as u64
+    }
+
     fn unmap(&mut self) -> Result<(), Error> {
         let Some(index) = NonNull::new(self.index as *mut std::ffi::c_void) else {
             return Ok(());
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 90a84ed2..dc4f9bf5 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -637,6 +637,14 @@ impl BackupEnvironment {
             }
         }
 
+        let writer_size = data.index.size();
+        if size != writer_size {
+            bail!(
+                "fixed writer '{}' close failed - unexpected size ({size} != {writer_size})",
+                data.name,
+            );
+        }
+
         let expected_csum = data.index.close()?;
         data.closed = true;
 
-- 
2.47.3

[PATCH v5 proxmox-backup 09/16] fix #3847: client: support fifo pipe inputs for images

[Robert Obkircher]

Accept fifo files as inputs for image backups. The unknown size is
represented in the UploadOptions using a new IndexType enum where the
total size for the Fixed variant is now optional.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-client/src/backup_writer.rs   | 38 ++++++++++++++++++++++---------
 proxmox-backup-client/src/main.rs | 30 +++++++++++++-----------
 src/server/push.rs                | 11 +++++----
 3 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index dbd177d8..f33f1063 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -52,7 +52,17 @@ pub struct UploadOptions {
     pub previous_manifest: Option<Arc<BackupManifest>>,
     pub compress: bool,
     pub encrypt: bool,
-    pub fixed_size: Option<u64>,
+    pub index_type: IndexType,
+}
+
+/// Index type for upload options.
+#[derive(Default, Clone)]
+pub enum IndexType {
+    /// Dynamic chunking.
+    #[default]
+    Dynamic,
+    /// Fixed size chunking with optional image file size.
+    Fixed(Option<u64>),
 }
 
 struct ChunkUploadResponse {
@@ -292,11 +302,14 @@ impl BackupWriter {
         options: UploadOptions,
     ) -> Result<BackupStats, Error> {
         let mut param = json!({ "archive-name": archive_name });
-        let prefix = if let Some(size) = options.fixed_size {
-            param["size"] = size.into();
-            "fixed"
-        } else {
-            "dynamic"
+        let prefix = match options.index_type {
+            IndexType::Fixed(image_file_size) => {
+                if let Some(size) = image_file_size {
+                    param["size"] = size.into();
+                }
+                "fixed"
+            }
+            IndexType::Dynamic => "dynamic",
         };
 
         if options.encrypt && self.crypt_config.is_none() {
@@ -387,11 +400,14 @@ impl BackupWriter {
         let known_chunks = Arc::new(Mutex::new(HashSet::new()));
 
         let mut param = json!({ "archive-name": archive_name });
-        let prefix = if let Some(size) = options.fixed_size {
-            param["size"] = size.into();
-            "fixed"
-        } else {
-            "dynamic"
+        let prefix = match options.index_type {
+            IndexType::Fixed(image_file_size) => {
+                if let Some(size) = image_file_size {
+                    param["size"] = size.into();
+                }
+                "fixed"
+            }
+            IndexType::Dynamic => "dynamic",
         };
 
         if options.encrypt && self.crypt_config.is_none() {
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 999e5020..7fc711fd 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -46,7 +46,7 @@ use pbs_client::tools::{
 use pbs_client::{
     delete_ticket_info, parse_backup_specification, view_task_result, BackupDetectionMode,
     BackupReader, BackupRepository, BackupSpecificationType, BackupStats, BackupWriter,
-    BackupWriterOptions, ChunkStream, FixedChunkStream, HttpClient, InjectionData,
+    BackupWriterOptions, ChunkStream, FixedChunkStream, HttpClient, IndexType, InjectionData,
     PxarBackupStream, RemoteChunkReader, UploadOptions, BACKUP_SOURCE_SCHEMA,
 };
 use pbs_datastore::catalog::{BackupCatalogWriter, CatalogReader, CatalogWriter};
@@ -205,7 +205,7 @@ async fn backup_directory<P: AsRef<Path>>(
     pxar_create_options: pbs_client::pxar::PxarCreateOptions,
     upload_options: UploadOptions,
 ) -> Result<(BackupStats, Option<BackupStats>), Error> {
-    if upload_options.fixed_size.is_some() {
+    if let IndexType::Fixed(_) = upload_options.index_type {
         bail!("cannot backup directory with fixed chunk size!");
     }
 
@@ -295,7 +295,7 @@ async fn backup_image<P: AsRef<Path>>(
 
     let stream = FixedChunkStream::new(stream, chunk_size.unwrap_or(4 * 1024 * 1024));
 
-    if upload_options.fixed_size.is_none() {
+    if let IndexType::Dynamic = upload_options.index_type {
         bail!("cannot backup image with dynamic chunk size!");
     }
 
@@ -859,15 +859,17 @@ async fn create_backup(
                 upload_list.push((BackupSpecificationType::PXAR, filename, target, "didx", 0));
             }
             BackupSpecificationType::IMAGE => {
-                if !(file_type.is_file() || file_type.is_block_device()) {
-                    bail!("got unexpected file type (expected file or block device)");
-                }
-
-                let size = image_size(&PathBuf::from(&filename))?;
-
-                if size == 0 {
-                    bail!("got zero-sized file '{}'", filename);
-                }
+                let size = if file_type.is_file() || file_type.is_block_device() {
+                    let size = image_size(&PathBuf::from(&filename))?;
+                    if size == 0 {
+                        bail!("got zero-sized file '{}'", filename);
+                    }
+                    size
+                } else if file_type.is_fifo() {
+                    0
+                } else {
+                    bail!("got unexpected file type (expected file, block device, or fifo");
+                };
 
                 upload_list.push((
                     BackupSpecificationType::IMAGE,
@@ -1191,9 +1193,11 @@ async fn create_backup(
             (BackupSpecificationType::IMAGE, false) => {
                 log_file("image", &filename, target.as_ref());
 
+                // 0 means fifo pipe with unknown size
+                let image_file_size = (size != 0).then_some(size);
                 let upload_options = UploadOptions {
                     previous_manifest: previous_manifest.clone(),
-                    fixed_size: Some(size),
+                    index_type: IndexType::Fixed(image_file_size),
                     compress: true,
                     encrypt: crypto.mode == CryptMode::Encrypt,
                 };
diff --git a/src/server/push.rs b/src/server/push.rs
index d7884fce..b1b41297 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -17,7 +17,8 @@ use pbs_api_types::{
     PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE,
 };
 use pbs_client::{
-    BackupRepository, BackupWriter, BackupWriterOptions, HttpClient, MergedChunkInfo, UploadOptions,
+    BackupRepository, BackupWriter, BackupWriterOptions, HttpClient, IndexType, MergedChunkInfo,
+    UploadOptions,
 };
 use pbs_config::CachedUserInfo;
 use pbs_datastore::data_blob::ChunkInfo;
@@ -917,7 +918,7 @@ pub(crate) async fn push_snapshot(
                         index,
                         chunk_reader,
                         &backup_writer,
-                        None,
+                        IndexType::Dynamic,
                         known_chunks.clone(),
                     )
                     .await?;
@@ -944,7 +945,7 @@ pub(crate) async fn push_snapshot(
                         index,
                         chunk_reader,
                         &backup_writer,
-                        Some(size),
+                        IndexType::Fixed(Some(size)),
                         known_chunks.clone(),
                     )
                     .await?;
@@ -1002,7 +1003,7 @@ async fn push_index(
     index: impl IndexFile + Send + 'static,
     chunk_reader: Arc<dyn AsyncReadChunk>,
     backup_writer: &BackupWriter,
-    size: Option<u64>,
+    index_type: IndexType,
     known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
 ) -> Result<SyncStats, Error> {
     let (upload_channel_tx, upload_channel_rx) = mpsc::channel(20);
@@ -1048,7 +1049,7 @@ async fn push_index(
     let upload_options = UploadOptions {
         compress: true,
         encrypt: false,
-        fixed_size: size,
+        index_type,
         ..UploadOptions::default()
     };
 
-- 
2.47.3

[PATCH v5 proxmox-backup 10/16] client: treat minus sign as stdin

[Robert Obkircher]

Treat "-" as an alias for "/dev/stdin". If there is an actual file
with that name it can still be read via "./-".

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-backup-client/src/main.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 7fc711fd..37878b01 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -845,6 +845,13 @@ async fn create_backup(
         }
         target_set.insert(target.clone());
 
+        // one can still use ./- to refer to an actual file with that name
+        let filename = if filename == "-" {
+            String::from("/dev/stdin")
+        } else {
+            filename
+        };
+
         use std::os::unix::fs::FileTypeExt;
 
         let metadata = std::fs::metadata(&filename)
-- 
2.47.3

[PATCH v5 proxmox-backup 11/16] datastore: combine public FixedIndexWriter methods into add_chunk.

[Robert Obkircher]

These operations were always performed together anyway, so it makes
sense to simplify the public api.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 24 +++++++++++++++++++++---
 src/api2/backup/environment.rs   |  6 +-----
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index c4fe899d..f50f4517 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -380,7 +380,7 @@ impl FixedIndexWriter {
     /// The size also becomes fixed as soon as it is no longer divisible
     /// by the block size, to ensure that only the last block can be
     /// smaller.
-    pub fn grow_to_size(&mut self, requested_size: usize) -> Result<(), Error> {
+    fn grow_to_size(&mut self, requested_size: usize) -> Result<(), Error> {
         if self.size < requested_size {
             if !self.growable_size {
                 bail!("refusing to resize from {} to {requested_size}", self.size);
@@ -467,7 +467,7 @@ impl FixedIndexWriter {
         Ok(index_csum)
     }
 
-    pub fn check_chunk_alignment(&self, offset: usize, chunk_len: usize) -> Result<usize, Error> {
+    fn check_chunk_alignment(&self, offset: usize, chunk_len: usize) -> Result<usize, Error> {
         if offset < chunk_len {
             bail!("got chunk with small offset ({} < {}", offset, chunk_len);
         }
@@ -497,7 +497,7 @@ impl FixedIndexWriter {
         Ok(pos / self.chunk_size)
     }
 
-    pub fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
+    fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
         if index >= self.index_length {
             bail!(
                 "add digest failed - index out of range ({} >= {})",
@@ -519,6 +519,24 @@ impl FixedIndexWriter {
         Ok(())
     }
 
+    /// Write the digest of a chunk into this index file.
+    ///
+    /// The `start` and `size` parameters encode the range of
+    /// content that is backed up. It is verified that `start` is
+    /// aligned and that only the last chunk may be smaller.
+    ///
+    /// If this writer has been created without a fixed size, the
+    /// index capacity and content size are increased automatically
+    /// until an incomplete chunk is encountered.
+    pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
+        let Some(end) = start.checked_add(size.into()) else {
+            bail!("add_chunk: start and size are too large: {start}+{size}");
+        };
+        self.grow_to_size(end as usize)?;
+        let idx = self.check_chunk_alignment(end as usize, size as usize)?;
+        self.add_digest(idx, digest)
+    }
+
     pub fn clone_data_from(&mut self, reader: &FixedIndexReader) -> Result<(), Error> {
         if self.growable_size {
             bail!("reusing the index is only supported with known input size");
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index dc4f9bf5..04c5bf84 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -442,14 +442,10 @@ impl BackupEnvironment {
             );
         }
 
-        let end = (offset as usize) + (size as usize);
-        data.index.grow_to_size(end)?;
-        let idx = data.index.check_chunk_alignment(end, size as usize)?;
+        data.index.add_chunk(offset, size, digest)?;
 
         data.chunk_count += 1;
 
-        data.index.add_digest(idx, digest)?;
-
         Ok(())
     }
 
-- 
2.47.3

[PATCH v5 proxmox-backup 12/16] datastore: use u64 instead of usize for fidx writer content size

[Robert Obkircher]

The file format is independent of the systems pointer size, so it
makes sense to use a fixed size type.

The chunk size is also a u64 to avoid casts and because that is what
is stored in the header anyway. Most other places limit it to u32, but
widening shouldn't be an issue.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   |  4 +-
 pbs-datastore/src/fixed_index.rs | 71 ++++++++++++++++----------------
 src/api2/backup/environment.rs   |  8 ++--
 src/api2/backup/mod.rs           |  8 ++--
 4 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 694a5dd0..69bde917 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -695,8 +695,8 @@ impl DataStore {
     pub fn create_fixed_writer<P: AsRef<Path>>(
         &self,
         filename: P,
-        size: Option<usize>,
-        chunk_size: usize,
+        size: Option<u64>,
+        chunk_size: u64,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
         FixedIndexWriter::create(full_path, size, chunk_size)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index f50f4517..753dd5a5 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -214,8 +214,8 @@ pub struct FixedIndexWriter {
     file: File,
     filename: PathBuf,
     tmp_filename: PathBuf,
-    chunk_size: usize,
-    size: usize,
+    chunk_size: u64,
+    size: u64,
     index_length: usize,
     index_capacity: usize,
     index: *mut u8,
@@ -252,8 +252,8 @@ impl FixedIndexWriter {
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
         full_path: impl Into<PathBuf>,
-        known_size: Option<usize>,
-        chunk_size: usize,
+        known_size: Option<u64>,
+        chunk_size: u64,
     ) -> Result<Self, Error> {
         let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
@@ -291,10 +291,13 @@ impl FixedIndexWriter {
 
         file.write_all(&buffer)?;
 
-        let (index_length, index_capacity) = known_size
-            .map(|s| s.div_ceil(chunk_size))
-            .map(|len| (len, len))
-            .unwrap_or((0, Self::INITIAL_CAPACITY));
+        let (index_length, index_capacity) = match known_size {
+            Some(s) => {
+                let len = s.div_ceil(chunk_size).try_into()?;
+                (len, len)
+            }
+            None => (0, Self::INITIAL_CAPACITY),
+        };
 
         let index_size = index_capacity * 32;
         nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
@@ -380,13 +383,13 @@ impl FixedIndexWriter {
     /// The size also becomes fixed as soon as it is no longer divisible
     /// by the block size, to ensure that only the last block can be
     /// smaller.
-    fn grow_to_size(&mut self, requested_size: usize) -> Result<(), Error> {
+    fn grow_to_size(&mut self, requested_size: u64) -> Result<(), Error> {
         if self.size < requested_size {
             if !self.growable_size {
                 bail!("refusing to resize from {} to {requested_size}", self.size);
             }
-            let new_len = requested_size.div_ceil(self.chunk_size);
-            if new_len * self.chunk_size != requested_size {
+            let new_len = requested_size.div_ceil(self.chunk_size).try_into()?;
+            if new_len as u64 * self.chunk_size != requested_size {
                 // not a full chunk, so this must be the last one
                 self.growable_size = false;
                 self.set_index_capacity_or_unmap(new_len)?;
@@ -410,7 +413,7 @@ impl FixedIndexWriter {
 
     /// The current total size of the referenced content.
     pub fn size(&self) -> u64 {
-        self.size as u64
+        self.size
     }
 
     fn unmap(&mut self) -> Result<(), Error> {
@@ -467,12 +470,10 @@ impl FixedIndexWriter {
         Ok(index_csum)
     }
 
-    fn check_chunk_alignment(&self, offset: usize, chunk_len: usize) -> Result<usize, Error> {
-        if offset < chunk_len {
+    fn check_chunk_alignment(&self, offset: u64, chunk_len: u64) -> Result<usize, Error> {
+        let Some(pos) = offset.checked_sub(chunk_len) else {
             bail!("got chunk with small offset ({} < {}", offset, chunk_len);
-        }
-
-        let pos = offset - chunk_len;
+        };
 
         if offset > self.size {
             bail!("chunk data exceeds size ({} >= {})", offset, self.size);
@@ -494,7 +495,7 @@ impl FixedIndexWriter {
             bail!("got unaligned chunk (pos = {})", pos);
         }
 
-        Ok(pos / self.chunk_size)
+        Ok((pos / self.chunk_size) as usize)
     }
 
     fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
@@ -528,12 +529,12 @@ impl FixedIndexWriter {
     /// If this writer has been created without a fixed size, the
     /// index capacity and content size are increased automatically
     /// until an incomplete chunk is encountered.
-    pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
-        let Some(end) = start.checked_add(size.into()) else {
+    pub fn add_chunk(&mut self, start: u64, size: u64, digest: &[u8; 32]) -> Result<(), Error> {
+        let Some(end) = start.checked_add(size) else {
             bail!("add_chunk: start and size are too large: {start}+{size}");
         };
-        self.grow_to_size(end as usize)?;
-        let idx = self.check_chunk_alignment(end as usize, size as usize)?;
+        self.grow_to_size(end)?;
+        let idx = self.check_chunk_alignment(end, size)?;
         self.add_digest(idx, digest)
     }
 
@@ -542,7 +543,7 @@ impl FixedIndexWriter {
             bail!("reusing the index is only supported with known input size");
         }
 
-        if self.chunk_size != reader.chunk_size {
+        if Ok(self.chunk_size) != reader.chunk_size.try_into() {
             bail!("can't reuse file with different chunk size");
         }
 
@@ -565,7 +566,7 @@ mod tests {
     use super::*;
     use crate::temp_test_dir::TempTestDir;
 
-    const CS: usize = 4096;
+    const CS: u64 = 4096;
 
     #[test]
     fn test_empty() {
@@ -611,7 +612,7 @@ mod tests {
 
         let initial = FixedIndexWriter::INITIAL_CAPACITY;
         let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
-        let expected = test_data(steps.last().unwrap() * CS);
+        let expected = test_data(*steps.last().unwrap() as u64 * CS);
 
         let mut begin = 0;
         for chunk_count in steps {
@@ -628,7 +629,7 @@ mod tests {
         w.close().unwrap();
         drop(w);
 
-        let size = expected.len() * CS;
+        let size = expected.len() as u64 * CS;
         check_with_reader(&path, size, &expected);
         compare_to_known_size_writer(&path, size, &expected);
     }
@@ -639,7 +640,7 @@ mod tests {
         let path = dir.join("test_grow_to_misaligned_size");
         let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
 
-        let size = (FixedIndexWriter::INITIAL_CAPACITY + 42) * CS - 1; // last is not full
+        let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 42) * CS - 1; // last is not full
         let expected = test_data(size);
 
         w.grow_to_size(size).unwrap();
@@ -661,8 +662,8 @@ mod tests {
     struct TestChunk {
         digest: [u8; 32],
         index: usize,
-        size: usize,
-        end: usize,
+        size: u64,
+        end: u64,
     }
 
     impl TestChunk {
@@ -675,7 +676,7 @@ mod tests {
         }
     }
 
-    fn test_data(size: usize) -> Vec<TestChunk> {
+    fn test_data(size: u64) -> Vec<TestChunk> {
         (0..size.div_ceil(CS))
             .map(|index| {
                 let mut digest = [0u8; 32];
@@ -690,24 +691,24 @@ mod tests {
                 };
                 TestChunk {
                     digest,
-                    index,
+                    index: index as usize,
                     size,
-                    end: index * CS + size,
+                    end: index as u64 * CS + size,
                 }
             })
             .collect()
     }
 
-    fn check_with_reader(path: &Path, size: usize, chunks: &[TestChunk]) {
+    fn check_with_reader(path: &Path, size: u64, chunks: &[TestChunk]) {
         let reader = FixedIndexReader::open(path).unwrap();
-        assert_eq!(size as u64, reader.index_bytes());
+        assert_eq!(size, reader.index_bytes());
         assert_eq!(chunks.len(), reader.index_count());
         for c in chunks {
             assert_eq!(&c.digest, reader.index_digest(c.index).unwrap());
         }
     }
 
-    fn compare_to_known_size_writer(file: &Path, size: usize, chunks: &[TestChunk]) {
+    fn compare_to_known_size_writer(file: &Path, size: u64, chunks: &[TestChunk]) {
         let mut path = file.to_path_buf();
         path.set_extension("reference");
         let mut w = FixedIndexWriter::create(&path, Some(size), CS).unwrap();
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 04c5bf84..9773a87a 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -67,7 +67,7 @@ struct DynamicWriterState {
 struct FixedWriterState {
     name: String,
     index: FixedIndexWriter,
-    size: Option<usize>,
+    size: Option<u64>,
     chunk_size: u32,
     chunk_count: u64,
     small_chunk_count: usize, // allow 0..1 small chunks (last chunk may be smaller)
@@ -349,7 +349,7 @@ impl BackupEnvironment {
         &self,
         index: FixedIndexWriter,
         name: String,
-        size: Option<usize>,
+        size: Option<u64>,
         chunk_size: u32,
         incremental: bool,
     ) -> Result<usize, Error> {
@@ -442,7 +442,7 @@ impl BackupEnvironment {
             );
         }
 
-        data.index.add_chunk(offset, size, digest)?;
+        data.index.add_chunk(offset, size.into(), digest)?;
 
         data.chunk_count += 1;
 
@@ -622,7 +622,7 @@ impl BackupEnvironment {
             }
 
             if let Some(known_size) = data.size {
-                if size != known_size as u64 {
+                if size != known_size {
                     bail!(
                         "fixed writer '{}' close failed - unexpected file size ({} != {})",
                         data.name,
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index c2822c18..55207e6c 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -480,7 +480,7 @@ fn create_fixed_index(
     let env: &BackupEnvironment = rpcenv.as_ref();
 
     let name = required_string_param(&param, "archive-name")?.to_owned();
-    let size = param["size"].as_u64().map(usize::try_from).transpose()?;
+    let size = param["size"].as_u64();
     let reuse_csum = param["reuse-csum"].as_str();
 
     let archive_name = name.clone();
@@ -528,13 +528,15 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
+    let mut writer = env
+        .datastore
+        .create_fixed_writer(&path, size, chunk_size.into())?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
     }
 
-    let wid = env.register_fixed_writer(writer, name, size, chunk_size as u32, incremental)?;
+    let wid = env.register_fixed_writer(writer, name, size, chunk_size, incremental)?;
 
     env.log(format!("created new fixed index {wid} ({path:?})"));
 
-- 
2.47.3

[PATCH v5 proxmox-backup 13/16] datastore: compute fidx file size with overflow checks

[Robert Obkircher]

Handle overflows and prevent potential inconsistencies by performing
the calcualtion in one place. This also serves as a preparation for
mmapping the entire file.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 753dd5a5..008339bb 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -300,7 +300,8 @@ impl FixedIndexWriter {
         };
 
         let index_size = index_capacity * 32;
-        nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
+        let file_size = Self::file_size(index_capacity)?;
+        nix::unistd::ftruncate(&file, file_size)?;
 
         let data = unsafe {
             nix::sys::mman::mmap(
@@ -332,6 +333,22 @@ impl FixedIndexWriter {
         })
     }
 
+    /// Computes the size of a fidx file containing `index_length`
+    /// chunk digests.
+    ///
+    /// Guarantees that the size fits into usize, isize and i64.
+    fn file_size(index_length: usize) -> Result<i64, Error> {
+        if index_length == 0 {
+            bail!("fidx file must have at least one chunk");
+        }
+        index_length
+            .checked_mul(32)
+            .and_then(|s| s.checked_add(size_of::<FixedIndexHeader>()))
+            .filter(|s| *s <= isize::MAX as usize)
+            .and_then(|s| i64::try_from(s).ok())
+            .ok_or_else(|| format_err!("fidx file size overflow for {index_length} chunks"))
+    }
+
     /// If this returns an error, the sizes may be out of sync,
     /// which is especially bad if the capacity was reduced.
     fn set_index_capacity(&mut self, new_capacity: usize) -> Result<(), Error> {
@@ -340,7 +357,7 @@ impl FixedIndexWriter {
         }
         let old_index_size = self.index_capacity * 32;
         let new_index_size = new_capacity * 32;
-        let new_file_size = (size_of::<FixedIndexHeader>() + new_index_size) as i64;
+        let new_file_size = Self::file_size(new_capacity)?;
 
         let index_addr = NonNull::new(self.index as *mut std::ffi::c_void).ok_or_else(|| {
             format_err!("Can't resize FixedIndexWriter index because the index pointer is null.")
@@ -443,11 +460,9 @@ impl FixedIndexWriter {
 
         self.unmap()?;
 
-        if self.index_length == 0 {
-            bail!("refusing to close empty fidx file {:?}", self.tmp_filename);
-        } else if self.index_length < self.index_capacity {
-            let file_size = size_of::<FixedIndexHeader>() + index_size;
-            nix::unistd::ftruncate(&self.file, file_size as i64)?;
+        if self.index_length < self.index_capacity {
+            let file_size = Self::file_size(self.index_length)?;
+            nix::unistd::ftruncate(&self.file, file_size)?;
             self.index_capacity = self.index_length;
         }
 
-- 
2.47.3

[PATCH v5 proxmox-backup 14/16] datastore: support writing fidx files on systems with larger page size

[Robert Obkircher]

A file can only be memory mapped at an offset which is a multiple of
the page size. Mapping from the start avoids this issue and simplifies
writes to the header.

The content size is now written unconditionally on close, because the
OS writes the entire page anyway to persist the checksum.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 121 ++++++++++++++-----------------
 1 file changed, 54 insertions(+), 67 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 008339bb..e8c14e5c 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -1,5 +1,4 @@
 use std::fs::File;
-use std::io::Write;
 use std::io::{Seek, SeekFrom};
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
@@ -210,6 +209,18 @@ impl IndexFile for FixedIndexReader {
     }
 }
 
+struct MmapPtr(NonNull<std::ffi::c_void>);
+
+impl MmapPtr {
+    fn header(&self) -> NonNull<FixedIndexHeader> {
+        self.0.cast::<FixedIndexHeader>()
+    }
+
+    fn index(&self) -> NonNull<u8> {
+        unsafe { self.0.byte_add(size_of::<FixedIndexHeader>()).cast::<u8>() }
+    }
+}
+
 pub struct FixedIndexWriter {
     file: File,
     filename: PathBuf,
@@ -218,11 +229,10 @@ pub struct FixedIndexWriter {
     size: u64,
     index_length: usize,
     index_capacity: usize,
-    index: *mut u8,
+    memory: Option<MmapPtr>,
     pub uuid: [u8; 16],
     pub ctime: i64,
     growable_size: bool,
-    write_size_on_close: bool,
 }
 
 // `index` is mmap()ed which cannot be thread-local so should be sendable
@@ -259,7 +269,7 @@ impl FixedIndexWriter {
         let mut tmp_path = full_path.clone();
         tmp_path.set_extension("tmp_fidx");
 
-        let mut file = std::fs::OpenOptions::new()
+        let file = std::fs::OpenOptions::new()
             .create(true)
             .truncate(true)
             .read(true)
@@ -278,19 +288,6 @@ impl FixedIndexWriter {
 
         let uuid = Uuid::generate();
 
-        let buffer = vec![0u8; header_size];
-        let header = unsafe { &mut *(buffer.as_ptr() as *mut FixedIndexHeader) };
-
-        header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0;
-        header.ctime = i64::to_le(ctime);
-        header.size = u64::to_le(size as u64);
-        header.chunk_size = u64::to_le(chunk_size as u64);
-        header.uuid = *uuid.as_bytes();
-
-        header.index_csum = [0u8; 32];
-
-        file.write_all(&buffer)?;
-
         let (index_length, index_capacity) = match known_size {
             Some(s) => {
                 let len = s.div_ceil(chunk_size).try_into()?;
@@ -299,23 +296,25 @@ impl FixedIndexWriter {
             None => (0, Self::INITIAL_CAPACITY),
         };
 
-        let index_size = index_capacity * 32;
         let file_size = Self::file_size(index_capacity)?;
         nix::unistd::ftruncate(&file, file_size)?;
 
-        let data = unsafe {
+        let memory = MmapPtr(unsafe {
             nix::sys::mman::mmap(
                 None,
-                std::num::NonZeroUsize::new(index_size)
-                    .ok_or_else(|| format_err!("invalid index size"))?,
+                std::num::NonZeroUsize::new(file_size as usize).expect("has header"),
                 nix::sys::mman::ProtFlags::PROT_READ | nix::sys::mman::ProtFlags::PROT_WRITE,
                 nix::sys::mman::MapFlags::MAP_SHARED,
                 &file,
-                header_size as i64,
+                0,
             )
-        }?
-        .as_ptr()
-        .cast::<u8>();
+        }?);
+
+        let header = unsafe { memory.header().as_mut() };
+        header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0;
+        header.ctime = i64::to_le(ctime);
+        header.chunk_size = u64::to_le(chunk_size);
+        header.uuid = *uuid.as_bytes();
 
         Ok(Self {
             file,
@@ -325,11 +324,10 @@ impl FixedIndexWriter {
             size,
             index_length,
             index_capacity,
-            index: data,
+            memory: Some(memory),
             ctime,
             uuid: *uuid.as_bytes(),
             growable_size: known_size.is_none(),
-            write_size_on_close: known_size.is_none(),
         })
     }
 
@@ -355,27 +353,26 @@ impl FixedIndexWriter {
         if new_capacity == self.index_capacity {
             return Ok(());
         }
-        let old_index_size = self.index_capacity * 32;
-        let new_index_size = new_capacity * 32;
-        let new_file_size = Self::file_size(new_capacity)?;
+        let old_size = Self::file_size(self.index_capacity)?;
+        let new_size = Self::file_size(new_capacity)?;
 
-        let index_addr = NonNull::new(self.index as *mut std::ffi::c_void).ok_or_else(|| {
-            format_err!("Can't resize FixedIndexWriter index because the index pointer is null.")
-        })?;
+        let Some(MmapPtr(index_addr)) = self.memory else {
+            bail!("Can't resize unmapped FixedIndexWriter");
+        };
 
-        nix::unistd::ftruncate(&self.file, new_file_size)?;
+        nix::unistd::ftruncate(&self.file, new_size)?;
 
         let new_index = unsafe {
             nix::sys::mman::mremap(
                 index_addr,
-                old_index_size,
-                new_index_size,
+                old_size as usize,
+                new_size as usize,
                 nix::sys::mman::MRemapFlags::MREMAP_MAYMOVE,
                 None,
             )
         }?;
 
-        self.index = new_index.as_ptr().cast::<u8>();
+        self.memory = Some(MmapPtr(new_index));
         self.index_capacity = new_capacity;
         Ok(())
     }
@@ -388,7 +385,7 @@ impl FixedIndexWriter {
                 "failed to resize index capacity from {} to {new_capacity} with backing file: {:?}",
                 self.index_capacity, self.tmp_filename
             );
-            assert!(self.index.is_null(), "{message} {unmap_result:?}");
+            assert!(self.memory.is_none(), "{message} {unmap_result:?}");
             e.context(message)
         })
     }
@@ -434,30 +431,32 @@ impl FixedIndexWriter {
     }
 
     fn unmap(&mut self) -> Result<(), Error> {
-        let Some(index) = NonNull::new(self.index as *mut std::ffi::c_void) else {
-            return Ok(());
-        };
-
-        let index_size = self.index_capacity * 32;
-
-        if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
-            bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
+        if let Some(ptr) = self.memory.take() {
+            let len = Self::file_size(self.index_capacity).expect(
+                "this is the capacity that didn't cause an overflow when the memory was mapped",
+            );
+            if let Err(err) = unsafe { nix::sys::mman::munmap(ptr.0, len as usize) } {
+                bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
+            }
         }
-
-        self.index = std::ptr::null_mut();
-
         Ok(())
     }
 
     pub fn close(&mut self) -> Result<[u8; 32], Error> {
-        if self.index.is_null() {
+        let Some(ptr) = &self.memory else {
             bail!("cannot close already closed index file.");
-        }
+        };
 
         let index_size = self.index_length * 32;
-        let data = unsafe { std::slice::from_raw_parts(self.index, index_size) };
+        let data = unsafe { std::slice::from_raw_parts(ptr.index().as_ptr(), index_size) };
         let index_csum = openssl::sha::sha256(data);
 
+        {
+            let header = unsafe { ptr.header().as_mut() };
+            header.index_csum = index_csum;
+            header.size = self.size.to_le();
+        }
+
         self.unmap()?;
 
         if self.index_length < self.index_capacity {
@@ -466,18 +465,6 @@ impl FixedIndexWriter {
             self.index_capacity = self.index_length;
         }
 
-        let csum_offset = std::mem::offset_of!(FixedIndexHeader, index_csum);
-        self.file.seek(SeekFrom::Start(csum_offset as u64))?;
-        self.file.write_all(&index_csum)?;
-
-        if self.write_size_on_close {
-            let size_offset = std::mem::offset_of!(FixedIndexHeader, size);
-            self.file.seek(SeekFrom::Start(size_offset as u64))?;
-            self.file.write_all(&(self.size as u64).to_le_bytes())?;
-        }
-
-        self.file.flush()?;
-
         if let Err(err) = std::fs::rename(&self.tmp_filename, &self.filename) {
             bail!("Atomic rename file {:?} failed - {}", self.filename, err);
         }
@@ -522,13 +509,13 @@ impl FixedIndexWriter {
             );
         }
 
-        if self.index.is_null() {
+        let Some(ptr) = &self.memory else {
             bail!("cannot write to closed index file.");
-        }
+        };
 
         let index_pos = index * 32;
         unsafe {
-            let dst = self.index.add(index_pos);
+            let dst = ptr.index().as_ptr().add(index_pos);
             dst.copy_from_nonoverlapping(digest.as_ptr(), 32);
         }
 
-- 
2.47.3

[PATCH v5 proxmox-backup 15/16] datastore: FixedIndexWriter: switch public chunk_size to u32

[Robert Obkircher]

I'm not sure if this patch should be applied.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   | 4 ++--
 pbs-datastore/src/fixed_index.rs | 6 +++---
 src/api2/backup/environment.rs   | 2 +-
 src/api2/backup/mod.rs           | 4 +---
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 69bde917..98eab18d 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -696,10 +696,10 @@ impl DataStore {
         &self,
         filename: P,
         size: Option<u64>,
-        chunk_size: u64,
+        chunk_size: u32,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
-        FixedIndexWriter::create(full_path, size, chunk_size)
+        FixedIndexWriter::create(full_path, size, chunk_size.into())
     }
 
     pub fn open_fixed_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index e8c14e5c..3fa146f8 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -531,12 +531,12 @@ impl FixedIndexWriter {
     /// If this writer has been created without a fixed size, the
     /// index capacity and content size are increased automatically
     /// until an incomplete chunk is encountered.
-    pub fn add_chunk(&mut self, start: u64, size: u64, digest: &[u8; 32]) -> Result<(), Error> {
-        let Some(end) = start.checked_add(size) else {
+    pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
+        let Some(end) = start.checked_add(size.into()) else {
             bail!("add_chunk: start and size are too large: {start}+{size}");
         };
         self.grow_to_size(end)?;
-        let idx = self.check_chunk_alignment(end, size)?;
+        let idx = self.check_chunk_alignment(end, size.into())?;
         self.add_digest(idx, digest)
     }
 
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 9773a87a..9645f6de 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -442,7 +442,7 @@ impl BackupEnvironment {
             );
         }
 
-        data.index.add_chunk(offset, size.into(), digest)?;
+        data.index.add_chunk(offset, size, digest)?;
 
         data.chunk_count += 1;
 
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 55207e6c..4d0846ca 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -528,9 +528,7 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env
-        .datastore
-        .create_fixed_writer(&path, size, chunk_size.into())?;
+    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
-- 
2.47.3

[PATCH v5 proxmox-backup 16/16] datastore: FixedIndexWriter: switch internal chunk_size to u32

[Robert Obkircher]

I don't really like this change becasue it introduces quite a few
conversions.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   |  2 +-
 pbs-datastore/src/fixed_index.rs | 43 ++++++++++++++++----------------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 98eab18d..631bdc30 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -699,7 +699,7 @@ impl DataStore {
         chunk_size: u32,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
-        FixedIndexWriter::create(full_path, size, chunk_size.into())
+        FixedIndexWriter::create(full_path, size, chunk_size)
     }
 
     pub fn open_fixed_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 3fa146f8..ba6874fe 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -225,7 +225,7 @@ pub struct FixedIndexWriter {
     file: File,
     filename: PathBuf,
     tmp_filename: PathBuf,
-    chunk_size: u64,
+    chunk_size: u32,
     size: u64,
     index_length: usize,
     index_capacity: usize,
@@ -263,7 +263,7 @@ impl FixedIndexWriter {
     pub fn create(
         full_path: impl Into<PathBuf>,
         known_size: Option<u64>,
-        chunk_size: u64,
+        chunk_size: u32,
     ) -> Result<Self, Error> {
         let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
@@ -290,7 +290,7 @@ impl FixedIndexWriter {
 
         let (index_length, index_capacity) = match known_size {
             Some(s) => {
-                let len = s.div_ceil(chunk_size).try_into()?;
+                let len = s.div_ceil(chunk_size.into()).try_into()?;
                 (len, len)
             }
             None => (0, Self::INITIAL_CAPACITY),
@@ -313,7 +313,7 @@ impl FixedIndexWriter {
         let header = unsafe { memory.header().as_mut() };
         header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0;
         header.ctime = i64::to_le(ctime);
-        header.chunk_size = u64::to_le(chunk_size);
+        header.chunk_size = u64::to_le(chunk_size.into());
         header.uuid = *uuid.as_bytes();
 
         Ok(Self {
@@ -402,8 +402,9 @@ impl FixedIndexWriter {
             if !self.growable_size {
                 bail!("refusing to resize from {} to {requested_size}", self.size);
             }
-            let new_len = requested_size.div_ceil(self.chunk_size).try_into()?;
-            if new_len as u64 * self.chunk_size != requested_size {
+            let cs = u64::from(self.chunk_size);
+            let new_len = requested_size.div_ceil(cs).try_into()?;
+            if new_len as u64 * cs != requested_size {
                 // not a full chunk, so this must be the last one
                 self.growable_size = false;
                 self.set_index_capacity_or_unmap(new_len)?;
@@ -472,8 +473,8 @@ impl FixedIndexWriter {
         Ok(index_csum)
     }
 
-    fn check_chunk_alignment(&self, offset: u64, chunk_len: u64) -> Result<usize, Error> {
-        let Some(pos) = offset.checked_sub(chunk_len) else {
+    fn check_chunk_alignment(&self, offset: u64, chunk_len: u32) -> Result<usize, Error> {
+        let Some(pos) = offset.checked_sub(chunk_len.into()) else {
             bail!("got chunk with small offset ({} < {}", offset, chunk_len);
         };
 
@@ -493,11 +494,11 @@ impl FixedIndexWriter {
             );
         }
 
-        if pos & (self.chunk_size - 1) != 0 {
+        if pos & (u64::from(self.chunk_size) - 1) != 0 {
             bail!("got unaligned chunk (pos = {})", pos);
         }
 
-        Ok((pos / self.chunk_size) as usize)
+        Ok((pos / u64::from(self.chunk_size)) as usize)
     }
 
     fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
@@ -536,7 +537,7 @@ impl FixedIndexWriter {
             bail!("add_chunk: start and size are too large: {start}+{size}");
         };
         self.grow_to_size(end)?;
-        let idx = self.check_chunk_alignment(end, size.into())?;
+        let idx = self.check_chunk_alignment(end, size)?;
         self.add_digest(idx, digest)
     }
 
@@ -568,7 +569,7 @@ mod tests {
     use super::*;
     use crate::temp_test_dir::TempTestDir;
 
-    const CS: u64 = 4096;
+    const CS: u32 = 4096;
 
     #[test]
     fn test_empty() {
@@ -594,7 +595,7 @@ mod tests {
         let path = dir.join("test_single_partial_chunk");
         let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
 
-        let size = CS - 1;
+        let size = CS as u64 - 1;
         let expected = test_data(size);
         w.grow_to_size(size).unwrap();
         expected[0].add_to(&mut w);
@@ -614,7 +615,7 @@ mod tests {
 
         let initial = FixedIndexWriter::INITIAL_CAPACITY;
         let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
-        let expected = test_data(*steps.last().unwrap() as u64 * CS);
+        let expected = test_data(*steps.last().unwrap() as u64 * CS as u64);
 
         let mut begin = 0;
         for chunk_count in steps {
@@ -631,7 +632,7 @@ mod tests {
         w.close().unwrap();
         drop(w);
 
-        let size = expected.len() as u64 * CS;
+        let size = expected.len() as u64 * CS as u64;
         check_with_reader(&path, size, &expected);
         compare_to_known_size_writer(&path, size, &expected);
     }
@@ -642,7 +643,7 @@ mod tests {
         let path = dir.join("test_grow_to_misaligned_size");
         let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
 
-        let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 42) * CS - 1; // last is not full
+        let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 42) * CS as u64 - 1; // last is not full
         let expected = test_data(size);
 
         w.grow_to_size(size).unwrap();
@@ -664,7 +665,7 @@ mod tests {
     struct TestChunk {
         digest: [u8; 32],
         index: usize,
-        size: u64,
+        size: u32,
         end: u64,
     }
 
@@ -679,23 +680,23 @@ mod tests {
     }
 
     fn test_data(size: u64) -> Vec<TestChunk> {
-        (0..size.div_ceil(CS))
+        (0..size.div_ceil(CS.into()))
             .map(|index| {
                 let mut digest = [0u8; 32];
                 let i = &(index as u64).to_le_bytes();
                 for c in digest.chunks_mut(i.len()) {
                     c.copy_from_slice(i);
                 }
-                let size = if ((index + 1) * CS) <= size {
+                let size = if ((index + 1) * CS as u64) <= size {
                     CS
                 } else {
-                    size % CS
+                    (size % CS as u64) as u32
                 };
                 TestChunk {
                     digest,
                     index: index as usize,
                     size,
-                    end: index as u64 * CS + size,
+                    end: index as u64 * CS as u64 + size as u64,
                 }
             })
             .collect()
-- 
2.47.3

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Lukas Wagner]

On Fri Jan 30, 2026 at 5:45 PM CET, Robert Obkircher wrote:
> This will be used to test the FixedIndexWriter and chunk store
> creation.
>
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>  pbs-datastore/src/lib.rs           |  4 ++++
>  pbs-datastore/src/temp_test_dir.rs | 33 ++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 pbs-datastore/src/temp_test_dir.rs
>
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 1f7c54ae..b48ec93c 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -233,3 +233,7 @@ pub use local_chunk_reader::LocalChunkReader;
>  
>  mod local_datastore_lru_cache;
>  pub use local_datastore_lru_cache::LocalDatastoreLruCache;
> +
> +#[cfg(test)]
> +mod temp_test_dir;
> +
> diff --git a/pbs-datastore/src/temp_test_dir.rs b/pbs-datastore/src/temp_test_dir.rs
> new file mode 100644
> index 00000000..13e16c91
> --- /dev/null
> +++ b/pbs-datastore/src/temp_test_dir.rs
> @@ -0,0 +1,33 @@
> +use std::convert::From;
> +use std::env;
> +use std::path::{Path, PathBuf};
> +
> +/// A temporary directory for tests that is removed on drop.
> +/// Deletion is best-effort and errors are silently ignored.
> +pub struct TempTestDir(PathBuf);
> +
> +impl TempTestDir {
> +    /// Create an empty directory.
> +    pub fn new() -> Self {
> +        Self(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
> +    }
> +}
> +
> +impl std::ops::Deref for TempTestDir {
> +    type Target = Path;
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
> +
> +impl Drop for TempTestDir {
> +    fn drop(&mut self) {
> +        let _ = std::fs::remove_dir_all(&self.0);
> +    }
> +}
> +
> +impl<'a> From<&'a TempTestDir> for PathBuf {
> +    fn from(temp: &'a TempTestDir) -> Self {
> +        temp.0.clone()
> +    }
> +}


Hey Robert,

We do already have a very similar helper in the PDM repository [1].
Maybe it would sense move either this or the PDM version (or some hybrid)
into a shared crate in proxmox.git?

I would suggest creating a new crate for commonly used test-only
helpers (proxmox-test-support, proxmox-test-helpers, proxmox-test-util,
maybe you have a better suggestion?)

What do you think?

[1] https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=server/src/test_support/temp.rs;h=a93c914d9efcc60de3bb40bc79610abc98571b7f;hb=HEAD

Re: [PATCH v5 proxmox-backup 01/16] datastore: remove Arc<ChunkStore> from FixedIndexWriter

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> The ChunkStore reference can be safely removed, because it was only
> needed for the base directory. As discussed previously in the context
> of the DynamicIndexWriter [1], it is the callers responsibility to
> ensure that the writer doesn't outlive any locks.
> 
> This simplifies testing the writer, which would otherwise require a
> workaround to avoid creating 65k directories.
> 
> [1] https://lore.proxmox.com/pbs-devel/1761822027.sm41v71zmk.astroid@yuna.none/
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

Changes look good to me, patch could already be applied IMO:

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

Re: [PATCH v5 proxmox-backup 02/16] datastore: remove Arc<ChunkStore> from DynamicIndexWriter

[Christian Ebner]

On 1/30/26 5:46 PM, Robert Obkircher wrote:
> Remain consistent with the FixedIndexWriter and make it easier to add
> tests in the future.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

Changes look good to me and also this patch might be applied already. 
Only thing which could be improved a bit if a new version is being send 
anyways is to explicitly mention the same rational as on the previous 
patch (making it easier to understand the change when looking at the 
commit history in the future).

Consider:

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

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Christian Ebner]

One comment inline.

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> This will be used to test the FixedIndexWriter and chunk store
> creation.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-datastore/src/lib.rs           |  4 ++++
>   pbs-datastore/src/temp_test_dir.rs | 33 ++++++++++++++++++++++++++++++
>   2 files changed, 37 insertions(+)
>   create mode 100644 pbs-datastore/src/temp_test_dir.rs
> 
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 1f7c54ae..b48ec93c 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -233,3 +233,7 @@ pub use local_chunk_reader::LocalChunkReader;
>   
>   mod local_datastore_lru_cache;
>   pub use local_datastore_lru_cache::LocalDatastoreLruCache;
> +
> +#[cfg(test)]
> +mod temp_test_dir;
> +
> diff --git a/pbs-datastore/src/temp_test_dir.rs b/pbs-datastore/src/temp_test_dir.rs
> new file mode 100644
> index 00000000..13e16c91
> --- /dev/null
> +++ b/pbs-datastore/src/temp_test_dir.rs
> @@ -0,0 +1,33 @@
> +use std::convert::From;
> +use std::env;
> +use std::path::{Path, PathBuf};
> +
> +/// A temporary directory for tests that is removed on drop.
> +/// Deletion is best-effort and errors are silently ignored.
> +pub struct TempTestDir(PathBuf);
> +
> +impl TempTestDir {
> +    /// Create an empty directory.

Might be worth to add that this directory is created at the location 
`TMPDIR` points to or falls back to `/tmp` with all the implications of 
that afaiu from 
https://doc.rust-lang.org/std/env/fn.temp_dir.html#platform-specific-behavior

> +    pub fn new() -> Self {
> +        Self(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
> +    }
> +}
> +
> +impl std::ops::Deref for TempTestDir {
> +    type Target = Path;
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
> +
> +impl Drop for TempTestDir {
> +    fn drop(&mut self) {
> +        let _ = std::fs::remove_dir_all(&self.0);
> +    }
> +}
> +
> +impl<'a> From<&'a TempTestDir> for PathBuf {
> +    fn from(temp: &'a TempTestDir) -> Self {
> +        temp.0.clone()
> +    }
> +}

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Robert Obkircher]

On 2/2/26 09:30, Lukas Wagner wrote:
> On Fri Jan 30, 2026 at 5:45 PM CET, Robert Obkircher wrote:
>> This will be used to test the FixedIndexWriter and chunk store
>> creation.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>> ---
>>  pbs-datastore/src/lib.rs           |  4 ++++
>>  pbs-datastore/src/temp_test_dir.rs | 33 ++++++++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+)
>>  create mode 100644 pbs-datastore/src/temp_test_dir.rs
>>
>> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
>> index 1f7c54ae..b48ec93c 100644
>> --- a/pbs-datastore/src/lib.rs
>> +++ b/pbs-datastore/src/lib.rs
>> @@ -233,3 +233,7 @@ pub use local_chunk_reader::LocalChunkReader;
>>  
>>  mod local_datastore_lru_cache;
>>  pub use local_datastore_lru_cache::LocalDatastoreLruCache;
>> +
>> +#[cfg(test)]
>> +mod temp_test_dir;
>> +
>> diff --git a/pbs-datastore/src/temp_test_dir.rs b/pbs-datastore/src/temp_test_dir.rs
>> new file mode 100644
>> index 00000000..13e16c91
>> --- /dev/null
>> +++ b/pbs-datastore/src/temp_test_dir.rs
>> @@ -0,0 +1,33 @@
>> +use std::convert::From;
>> +use std::env;
>> +use std::path::{Path, PathBuf};
>> +
>> +/// A temporary directory for tests that is removed on drop.
>> +/// Deletion is best-effort and errors are silently ignored.
>> +pub struct TempTestDir(PathBuf);
>> +
>> +impl TempTestDir {
>> +    /// Create an empty directory.
>> +    pub fn new() -> Self {
>> +        Self(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
>> +    }
>> +}
>> +
>> +impl std::ops::Deref for TempTestDir {
>> +    type Target = Path;
>> +    fn deref(&self) -> &Self::Target {
>> +        &self.0
>> +    }
>> +}
>> +
>> +impl Drop for TempTestDir {
>> +    fn drop(&mut self) {
>> +        let _ = std::fs::remove_dir_all(&self.0);
>> +    }
>> +}
>> +
>> +impl<'a> From<&'a TempTestDir> for PathBuf {
>> +    fn from(temp: &'a TempTestDir) -> Self {
>> +        temp.0.clone()
>> +    }
>> +}
>
> Hey Robert,
>
> We do already have a very similar helper in the PDM repository [1].
> Maybe it would sense move either this or the PDM version (or some hybrid)
> into a shared crate in proxmox.git?
>
> I would suggest creating a new crate for commonly used test-only
> helpers (proxmox-test-support, proxmox-test-helpers, proxmox-test-util,
> maybe you have a better suggestion?)
>
> What do you think?
I've noticed this as well when I skimmed your RFC.

I'm not sure if combining unrelated things into a -test
crate is a good idea. Maybe a (proxmox-)tempfile crate
that is added to [dev-dependencies] would be slightly
nicer?

I've also considered adding it to proxmox_sys::fs::dir,
but it does seem a little out of place there.

>
> [1] https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=server/src/test_support/temp.rs;h=a93c914d9efcc60de3bb40bc79610abc98571b7f;hb=HEAD

Re: [PATCH v5 proxmox-backup 04/16] datastore: use temporary directory for chunk store test

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Use the systems tmp directory instead of the package root, because the
> latter might not always be writeable and because the created files
> weren't even ignored by git.
> 
> This reduced the tests runtime from 3.2 to 0.6 seconds.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

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

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Christian Ebner]

noticed just now that this also introduces a formatting error when 
running `cargo fmt --check`.

Re: [PATCH v5 proxmox-backup 05/16] datastore: support writing fidx files of unknown size

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Use mremap and ftruncate to support growable FixedIndexWriters. Grow
> exponentially from a small initial index size and truncate excessive
> capacity after encountering a non-full block or on close.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Robert Obkircher]

On 2/2/26 11:16, Christian Ebner wrote:
> noticed just now that this also introduces a formatting error when
> running `cargo fmt --check`. 

What exactly is the error?

It works on my machine, but there is a chance that I messed up my rustfmt
version when I installed rustup with the newest nightly toolchain to
run miri.
(I found out miri doesn't support file backed mmap).

I have:

~/dev/proxmox-backup$ git log --oneline --
pbs-datastore/src/temp_test_dir.rs
f157c39e datastore: add TempTestDir that is automatically deleted on drop

~/dev/proxmox-backup$ cargo fmt --version
rustfmt 1.8.0-nightly (de6d33c033 2026-01-28)

~/dev/proxmox-backup$ /usr/bin/cargo-fmt --version
rustfmt 1.8.0
~/dev/proxmox-backup$ cargo fmt --check
~/dev/proxmox-backup$ cargo fmt

~/dev/proxmox-backup$ git status
On branch pipe
nothing to commit, working tree clean

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Lukas Wagner]

On Mon Feb 2, 2026 at 11:12 AM CET, Robert Obkircher wrote:
>
> On 2/2/26 09:30, Lukas Wagner wrote:
>> On Fri Jan 30, 2026 at 5:45 PM CET, Robert Obkircher wrote:
>>> This will be used to test the FixedIndexWriter and chunk store
>>> creation.
>>>
>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>> ---
>>>  pbs-datastore/src/lib.rs           |  4 ++++
>>>  pbs-datastore/src/temp_test_dir.rs | 33 ++++++++++++++++++++++++++++++
>>>  2 files changed, 37 insertions(+)
>>>  create mode 100644 pbs-datastore/src/temp_test_dir.rs
>>>
>>> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
>>> index 1f7c54ae..b48ec93c 100644
>>> --- a/pbs-datastore/src/lib.rs
>>> +++ b/pbs-datastore/src/lib.rs
>>> @@ -233,3 +233,7 @@ pub use local_chunk_reader::LocalChunkReader;
>>>  
>>>  mod local_datastore_lru_cache;
>>>  pub use local_datastore_lru_cache::LocalDatastoreLruCache;
>>> +
>>> +#[cfg(test)]
>>> +mod temp_test_dir;
>>> +
>>> diff --git a/pbs-datastore/src/temp_test_dir.rs b/pbs-datastore/src/temp_test_dir.rs
>>> new file mode 100644
>>> index 00000000..13e16c91
>>> --- /dev/null
>>> +++ b/pbs-datastore/src/temp_test_dir.rs
>>> @@ -0,0 +1,33 @@
>>> +use std::convert::From;
>>> +use std::env;
>>> +use std::path::{Path, PathBuf};
>>> +
>>> +/// A temporary directory for tests that is removed on drop.
>>> +/// Deletion is best-effort and errors are silently ignored.
>>> +pub struct TempTestDir(PathBuf);
>>> +
>>> +impl TempTestDir {
>>> +    /// Create an empty directory.
>>> +    pub fn new() -> Self {
>>> +        Self(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
>>> +    }
>>> +}
>>> +
>>> +impl std::ops::Deref for TempTestDir {
>>> +    type Target = Path;
>>> +    fn deref(&self) -> &Self::Target {
>>> +        &self.0
>>> +    }
>>> +}
>>> +
>>> +impl Drop for TempTestDir {
>>> +    fn drop(&mut self) {
>>> +        let _ = std::fs::remove_dir_all(&self.0);
>>> +    }
>>> +}
>>> +
>>> +impl<'a> From<&'a TempTestDir> for PathBuf {
>>> +    fn from(temp: &'a TempTestDir) -> Self {
>>> +        temp.0.clone()
>>> +    }
>>> +}
>>
>> Hey Robert,
>>
>> We do already have a very similar helper in the PDM repository [1].
>> Maybe it would sense move either this or the PDM version (or some hybrid)
>> into a shared crate in proxmox.git?
>>
>> I would suggest creating a new crate for commonly used test-only
>> helpers (proxmox-test-support, proxmox-test-helpers, proxmox-test-util,
>> maybe you have a better suggestion?)
>>
>> What do you think?
> I've noticed this as well when I skimmed your RFC.
>
> I'm not sure if combining unrelated things into a -test
> crate is a good idea. Maybe a (proxmox-)tempfile crate
> that is added to [dev-dependencies] would be slightly
> nicer?

Also okay to me. Then I would make sure that this helper is
'production-ready' - so maybe log the error in case `drop` could not
clean up the directory, add support for `CreateOptions`, and also maybe
add a method that allows one to clean up manually, so that any potential
error can be handle by the application if needed (TmpDir from the
tempfile crate has a `close` method, something like that).

Maybe it could also make sense to move the tmpfile helpers from
proxmox-sys there, if possible (IIRC proxmox-sys uses the tempfile
helpers internally).

>
> I've also considered adding it to proxmox_sys::fs::dir,
> but it does seem a little out of place there.

Yeah, I wouldn't do that. proxmox-sys is pretty huge already and should
rather be broken apart into meaningful smaller crates than made bigger.

>
>>
>> [1] https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=server/src/test_support/temp.rs;h=a93c914d9efcc60de3bb40bc79610abc98571b7f;hb=HEAD

Re: [PATCH v5 proxmox-backup 06/16] datastore: test FixedIndexWriter

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Write fixed and dynamically sized fidx files to a temporary directory.
> Compare the resulting files directly (ignoring uuid and ctime bytes)
> and also read them back using the reader.
> 
> The chunk hashes are just dummy values that don't actually exist in a
> chunk store.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-datastore/src/fixed_index.rs | 170 +++++++++++++++++++++++++++++++
>   pbs-datastore/src/lib.rs         |   1 -
>   2 files changed, 170 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
> index bcb0cf94..0f87bf15 100644
> --- a/pbs-datastore/src/fixed_index.rs
> +++ b/pbs-datastore/src/fixed_index.rs
> @@ -534,3 +534,173 @@ impl FixedIndexWriter {
>           Ok(())
>       }
>   }
> +
> +#[cfg(test)]
> +mod tests {
> +    use std::fs;
> +
> +    use super::*;
> +    use crate::temp_test_dir::TempTestDir;
> +
> +    const CS: usize = 4096;
> +
> +    #[test]
> +    fn test_empty() {
> +        let dir = TempTestDir::new();
> +        let path = dir.join("test_empty");
> +        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
> +
> +        assert!(w.add_digest(0, &[1u8; 32]).is_err(), "out of bounds");
> +
> +        assert_eq!(0, w.size);
> +        assert_eq!(0, w.index_length(), "returns length, not capacity");
> +        assert_eq!(FixedIndexWriter::INITIAL_CAPACITY, w.index_capacity);
> +
> +        assert!(w.close().is_err(), "should refuse to create empty file");
> +
> +        drop(w);
> +        assert!(!fs::exists(path).unwrap());
> +    }
> +
> +    #[test]
> +    fn test_single_partial_chunk() {
> +        let dir = TempTestDir::new();
> +        let path = dir.join("test_single_partial_chunk");
> +        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
> +
> +        let size = CS - 1;
> +        let expected = test_data(size);
> +        w.grow_to_size(size).unwrap();
> +        expected[0].add_to(&mut w);
> +
> +        w.close().unwrap();
> +        drop(w);
> +
> +        check_with_reader(&path, size, &expected);
> +        compare_to_known_size_writer(&path, size, &expected);
> +    }
> +
> +    #[test]
> +    fn test_grow_to_multiples_of_chunk_size() {
> +        let dir = TempTestDir::new();
> +        let path = dir.join("test_grow_to_multiples_of_chunk_size");
> +        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
> +
> +        let initial = FixedIndexWriter::INITIAL_CAPACITY;
> +        let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
> +        let expected = test_data(steps.last().unwrap() * CS);
> +
> +        let mut begin = 0;
> +        for chunk_count in steps {
> +            let last = &expected[chunk_count - 1];
> +            w.grow_to_size(last.end).unwrap();
> +            assert_eq!(last.index + 1, w.index_length());
> +            assert!(w.add_digest(last.index + 1, &[1u8; 32]).is_err());
> +
> +            for c in expected[begin..chunk_count].iter().rev() {
> +                c.add_to(&mut w);
> +            }
> +            begin = chunk_count;
> +        }
> +        w.close().unwrap();
> +        drop(w);
> +
> +        let size = expected.len() * CS;
> +        check_with_reader(&path, size, &expected);
> +        compare_to_known_size_writer(&path, size, &expected);
> +    }
> +
> +    #[test]
> +    fn test_grow_to_misaligned_size() {
> +        let dir = TempTestDir::new();
> +        let path = dir.join("test_grow_to_misaligned_size");
> +        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
> +
> +        let size = (FixedIndexWriter::INITIAL_CAPACITY + 42) * CS - 1; // last is not full
> +        let expected = test_data(size);
> +
> +        w.grow_to_size(size).unwrap();
> +        assert!(w.grow_to_size(size + 1).is_err(), "size must be fixed now");
> +        assert_eq!(expected.len(), w.index_length());
> +        assert!(w.add_digest(expected.len(), &[1u8; 32]).is_err());
> +
> +        for c in expected.iter().rev() {
> +            c.add_to(&mut w);
> +        }
> +
> +        w.close().unwrap();
> +        drop(w);
> +
> +        check_with_reader(&path, size, &expected);
> +        compare_to_known_size_writer(&path, size, &expected);
> +    }
> +
> +    struct TestChunk {
> +        digest: [u8; 32],
> +        index: usize,
> +        size: usize,
> +        end: usize,
> +    }
> +
> +    impl TestChunk {
> +        fn add_to(&self, w: &mut FixedIndexWriter) {
> +            assert_eq!(
> +                self.index,
> +                w.check_chunk_alignment(self.end, self.size).unwrap()
> +            );
> +            w.add_digest(self.index, &self.digest).unwrap();
> +        }
> +    }
> +
> +    fn test_data(size: usize) -> Vec<TestChunk> {
> +        (0..size.div_ceil(CS))
> +            .map(|index| {
> +                let mut digest = [0u8; 32];
> +                let i = &(index as u64).to_le_bytes();
> +                for c in digest.chunks_mut(i.len()) {
> +                    c.copy_from_slice(i);
> +                }
> +                let size = if ((index + 1) * CS) <= size {
> +                    CS
> +                } else {
> +                    size % CS
> +                };
> +                TestChunk {
> +                    digest,
> +                    index,
> +                    size,
> +                    end: index * CS + size,
> +                }
> +            })
> +            .collect()
> +    }
> +
> +    fn check_with_reader(path: &Path, size: usize, chunks: &[TestChunk]) {
> +        let reader = FixedIndexReader::open(path).unwrap();
> +        assert_eq!(size as u64, reader.index_bytes());
> +        assert_eq!(chunks.len(), reader.index_count());
> +        for c in chunks {
> +            assert_eq!(&c.digest, reader.index_digest(c.index).unwrap());
> +        }
> +    }
> +
> +    fn compare_to_known_size_writer(file: &Path, size: usize, chunks: &[TestChunk]) {
> +        let mut path = file.to_path_buf();
> +        path.set_extension("reference");
> +        let mut w = FixedIndexWriter::create(&path, Some(size), CS).unwrap();
> +        for c in chunks {
> +            c.add_to(&mut w);
> +        }
> +        w.close().unwrap();
> +        drop(w);
> +
> +        let mut reference = fs::read(file).unwrap();
> +        let mut tested = fs::read(path).unwrap();
> +
> +        // ignore uuid and ctime
> +        reference[8..32].fill(0);
> +        tested[8..32].fill(0);
> +
> +        assert_eq!(reference, tested);
> +    }
> +}
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index b48ec93c..23a17d96 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -236,4 +236,3 @@ pub use local_datastore_lru_cache::LocalDatastoreLruCache;
>   
>   #[cfg(test)]
>   mod temp_test_dir;
> -

nit: unrelated hunk introduced by patch 0003, which should be fixed up 
there.

Apart from that, test look good to me.

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

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Christian Ebner]

On 2/2/26 11:49 AM, Robert Obkircher wrote:
> 
> On 2/2/26 11:16, Christian Ebner wrote:
>> noticed just now that this also introduces a formatting error when
>> running `cargo fmt --check`.
> 
> What exactly is the error?

See comment on patch 6, which cleans up the formatting error for 
following patches.

Re: [PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Robert Obkircher]

On 2/2/26 12:11, Christian Ebner wrote:
> On 2/2/26 11:49 AM, Robert Obkircher wrote:
>>
>> On 2/2/26 11:16, Christian Ebner wrote:
>>> noticed just now that this also introduces a formatting error when
>>> running `cargo fmt --check`.
>>
>> What exactly is the error?
>
> See comment on patch 6, which cleans up the formatting error for
> following patches. 

Sorry, that was a stupid mistake.

I'll create a script to cago fmt/check/clippy each patch in the future.

Re: [PATCH v5 proxmox-backup 07/16] api: backup: make fixed index file size optional

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Pass the optional size to the FixedIndexWriter to make it grow as
> necessary.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   src/api2/backup/environment.rs | 21 ++++++++++++---------
>   src/api2/backup/mod.rs         |  8 +++-----
>   2 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
> index bd9c5211..90a84ed2 100644
> --- a/src/api2/backup/environment.rs
> +++ b/src/api2/backup/environment.rs
> @@ -67,7 +67,7 @@ struct DynamicWriterState {
>   struct FixedWriterState {
>       name: String,
>       index: FixedIndexWriter,
> -    size: usize,
> +    size: Option<usize>,
>       chunk_size: u32,
>       chunk_count: u64,
>       small_chunk_count: usize, // allow 0..1 small chunks (last chunk may be smaller)
> @@ -349,7 +349,7 @@ impl BackupEnvironment {
>           &self,
>           index: FixedIndexWriter,
>           name: String,
> -        size: usize,
> +        size: Option<usize>,
>           chunk_size: u32,
>           incremental: bool,
>       ) -> Result<usize, Error> {
> @@ -443,6 +443,7 @@ impl BackupEnvironment {
>           }
>   
>           let end = (offset as usize) + (size as usize);
> +        data.index.grow_to_size(end)?;
>           let idx = data.index.check_chunk_alignment(end, size as usize)?;
>   
>           data.chunk_count += 1;
> @@ -624,13 +625,15 @@ impl BackupEnvironment {
>                   );
>               }
>   
> -            if size != (data.size as u64) {
> -                bail!(
> -                    "fixed writer '{}' close failed - unexpected file size ({} != {})",
> -                    data.name,
> -                    data.size,
> -                    size
> -                );
> +            if let Some(known_size) = data.size {
> +                if size != known_size as u64 {
> +                    bail!(
> +                        "fixed writer '{}' close failed - unexpected file size ({} != {})",
> +                        data.name,
> +                        known_size,
> +                        size,
> +                    );
> +                }
>               }
>           }
>   
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index c625a2dc..c2822c18 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -456,7 +456,7 @@ pub const API_METHOD_CREATE_FIXED_INDEX: ApiMethod = ApiMethod::new(
>               ("archive-name", false, &BACKUP_ARCHIVE_NAME_SCHEMA),
>               (
>                   "size",
> -                false,
> +                true,
>                   &IntegerSchema::new("File size.").minimum(1).schema()
>               ),
>               (
> @@ -480,7 +480,7 @@ fn create_fixed_index(
>       let env: &BackupEnvironment = rpcenv.as_ref();
>   
>       let name = required_string_param(&param, "archive-name")?.to_owned();
> -    let size = required_integer_param(&param, "size")? as usize;
> +    let size = param["size"].as_u64().map(usize::try_from).transpose()?;
>       let reuse_csum = param["reuse-csum"].as_str();

The fixed index writer does not allow for incremental backups, failing 
if it is requested.

question: should we allow to proceed the backup without failing by 
simply switching to a non incremental backup for those index writers 
where no size is specified, including a log warning that that a 
non-incremental mode is used?

Should be better than failing the whole backup snapshot in case of 
multiple archives with mixed inputs.

>   
>       let archive_name = name.clone();
> @@ -528,9 +528,7 @@ fn create_fixed_index(
>           reader = Some(index);
>       }
>   
> -    let mut writer = env
> -        .datastore
> -        .create_fixed_writer(&path, Some(size), chunk_size)?;
> +    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
>   
>       if let Some(reader) = reader {
>           writer.clone_data_from(&reader)?;

Re: [PATCH v5 proxmox-backup 08/16] api: verify fixed index writer size on close

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Compare it to the value submitted by the client to check that the
> resizing worked as expected.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

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

Re: [PATCH v5 proxmox-backup 09/16] fix #3847: client: support fifo pipe inputs for images

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Accept fifo files as inputs for image backups. The unknown size is
> represented in the UploadOptions using a new IndexType enum where the
> total size for the Fixed variant is now optional.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-client/src/backup_writer.rs   | 38 ++++++++++++++++++++++---------
>   proxmox-backup-client/src/main.rs | 30 +++++++++++++-----------
>   src/server/push.rs                | 11 +++++----
>   3 files changed, 50 insertions(+), 29 deletions(-)
> 
> diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
> index dbd177d8..f33f1063 100644
> --- a/pbs-client/src/backup_writer.rs
> +++ b/pbs-client/src/backup_writer.rs
> @@ -52,7 +52,17 @@ pub struct UploadOptions {
>       pub previous_manifest: Option<Arc<BackupManifest>>,
>       pub compress: bool,
>       pub encrypt: bool,
> -    pub fixed_size: Option<u64>,
> +    pub index_type: IndexType,
> +}
> +
> +/// Index type for upload options.
> +#[derive(Default, Clone)]
> +pub enum IndexType {
> +    /// Dynamic chunking.
> +    #[default]
> +    Dynamic,
> +    /// Fixed size chunking with optional image file size.
> +    Fixed(Option<u64>),
>   }
>   
>   struct ChunkUploadResponse {
> @@ -292,11 +302,14 @@ impl BackupWriter {
>           options: UploadOptions,
>       ) -> Result<BackupStats, Error> {
>           let mut param = json!({ "archive-name": archive_name });
> -        let prefix = if let Some(size) = options.fixed_size {
> -            param["size"] = size.into();
> -            "fixed"
> -        } else {
> -            "dynamic"
> +        let prefix = match options.index_type {
> +            IndexType::Fixed(image_file_size) => {
> +                if let Some(size) = image_file_size {
> +                    param["size"] = size.into();
> +                }
> +                "fixed"
> +            }
> +            IndexType::Dynamic => "dynamic",

this and the identical hunk below could be made a bit more readable by 
introducing a helper for the prefix and size on IndexType. Which would
also bundle the prefix generation into a single location. E.g.
```
diff --git a/pbs-client/src/backup_writer.rs 
b/pbs-client/src/backup_writer.rs
index f33f1063e..835e02ac2 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -65,6 +65,15 @@ pub enum IndexType {
      Fixed(Option<u64>),
  }

+impl IndexType {
+    fn to_prefix_and_size(&self) -> (&'static str, Option<u64>) {
+        match self {
+            IndexType::Fixed(size) => ("fixed", *size),
+            IndexType::Dynamic => ("dynamic", None),
+        }
+    }
+}
+
  struct ChunkUploadResponse {
      future: h2::client::ResponseFuture,
      size: usize,
@@ -302,15 +311,10 @@ impl BackupWriter {
          options: UploadOptions,
      ) -> Result<BackupStats, Error> {
          let mut param = json!({ "archive-name": archive_name });
-        let prefix = match options.index_type {
-            IndexType::Fixed(image_file_size) => {
-                if let Some(size) = image_file_size {
-                    param["size"] = size.into();
-                }
-                "fixed"
-            }
-            IndexType::Dynamic => "dynamic",
-        };
+        let (prefix, archive_size) = 
options.index_type.to_prefix_and_size();
+        if let Some(size) = archive_size {
+            param["size"] = size.into();
+        }

          if options.encrypt && self.crypt_config.is_none() {
              bail!("requested encryption without a crypt config");
```

>           };
>   
>           if options.encrypt && self.crypt_config.is_none() {
> @@ -387,11 +400,14 @@ impl BackupWriter {
>           let known_chunks = Arc::new(Mutex::new(HashSet::new()));
>   
>           let mut param = json!({ "archive-name": archive_name });
> -        let prefix = if let Some(size) = options.fixed_size {
> -            param["size"] = size.into();
> -            "fixed"
> -        } else {
> -            "dynamic"
> +        let prefix = match options.index_type {
> +            IndexType::Fixed(image_file_size) => {
> +                if let Some(size) = image_file_size {
> +                    param["size"] = size.into();
> +                }
> +                "fixed"
> +            }
> +            IndexType::Dynamic => "dynamic",
>           };
>   
>           if options.encrypt && self.crypt_config.is_none() {
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 999e5020..7fc711fd 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -46,7 +46,7 @@ use pbs_client::tools::{
>   use pbs_client::{
>       delete_ticket_info, parse_backup_specification, view_task_result, BackupDetectionMode,
>       BackupReader, BackupRepository, BackupSpecificationType, BackupStats, BackupWriter,
> -    BackupWriterOptions, ChunkStream, FixedChunkStream, HttpClient, InjectionData,
> +    BackupWriterOptions, ChunkStream, FixedChunkStream, HttpClient, IndexType, InjectionData,
>       PxarBackupStream, RemoteChunkReader, UploadOptions, BACKUP_SOURCE_SCHEMA,
>   };
>   use pbs_datastore::catalog::{BackupCatalogWriter, CatalogReader, CatalogWriter};
> @@ -205,7 +205,7 @@ async fn backup_directory<P: AsRef<Path>>(
>       pxar_create_options: pbs_client::pxar::PxarCreateOptions,
>       upload_options: UploadOptions,
>   ) -> Result<(BackupStats, Option<BackupStats>), Error> {
> -    if upload_options.fixed_size.is_some() {
> +    if let IndexType::Fixed(_) = upload_options.index_type {
>           bail!("cannot backup directory with fixed chunk size!");
>       }
>   
> @@ -295,7 +295,7 @@ async fn backup_image<P: AsRef<Path>>(
>   
>       let stream = FixedChunkStream::new(stream, chunk_size.unwrap_or(4 * 1024 * 1024));
>   
> -    if upload_options.fixed_size.is_none() {
> +    if let IndexType::Dynamic = upload_options.index_type {
>           bail!("cannot backup image with dynamic chunk size!");
>       }
>   
> @@ -859,15 +859,17 @@ async fn create_backup(
>                   upload_list.push((BackupSpecificationType::PXAR, filename, target, "didx", 0));
>               }
>               BackupSpecificationType::IMAGE => {
> -                if !(file_type.is_file() || file_type.is_block_device()) {
> -                    bail!("got unexpected file type (expected file or block device)");
> -                }
> -
> -                let size = image_size(&PathBuf::from(&filename))?;
> -
> -                if size == 0 {
> -                    bail!("got zero-sized file '{}'", filename);
> -                }
> +                let size = if file_type.is_file() || file_type.is_block_device() {
> +                    let size = image_size(&PathBuf::from(&filename))?;
> +                    if size == 0 {
> +                        bail!("got zero-sized file '{}'", filename);

nit: since touching this, the filename should be inlined into the format 
string.

> +                    }
> +                    size
> +                } else if file_type.is_fifo() {
> +                    0
> +                } else {
> +                    bail!("got unexpected file type (expected file, block device, or fifo");
> +                };
>   
>                   upload_list.push((
>                       BackupSpecificationType::IMAGE,
> @@ -1191,9 +1193,11 @@ async fn create_backup(
>               (BackupSpecificationType::IMAGE, false) => {
>                   log_file("image", &filename, target.as_ref());
>   
> +                // 0 means fifo pipe with unknown size
> +                let image_file_size = (size != 0).then_some(size);
>                   let upload_options = UploadOptions {
>                       previous_manifest: previous_manifest.clone(),
> -                    fixed_size: Some(size),
> +                    index_type: IndexType::Fixed(image_file_size),
>                       compress: true,
>                       encrypt: crypto.mode == CryptMode::Encrypt,
>                   };
> diff --git a/src/server/push.rs b/src/server/push.rs
> index d7884fce..b1b41297 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -17,7 +17,8 @@ use pbs_api_types::{
>       PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE,
>   };
>   use pbs_client::{
> -    BackupRepository, BackupWriter, BackupWriterOptions, HttpClient, MergedChunkInfo, UploadOptions,
> +    BackupRepository, BackupWriter, BackupWriterOptions, HttpClient, IndexType, MergedChunkInfo,
> +    UploadOptions,
>   };
>   use pbs_config::CachedUserInfo;
>   use pbs_datastore::data_blob::ChunkInfo;
> @@ -917,7 +918,7 @@ pub(crate) async fn push_snapshot(
>                           index,
>                           chunk_reader,
>                           &backup_writer,
> -                        None,
> +                        IndexType::Dynamic,
>                           known_chunks.clone(),
>                       )
>                       .await?;
> @@ -944,7 +945,7 @@ pub(crate) async fn push_snapshot(
>                           index,
>                           chunk_reader,
>                           &backup_writer,
> -                        Some(size),
> +                        IndexType::Fixed(Some(size)),
>                           known_chunks.clone(),
>                       )
>                       .await?;
> @@ -1002,7 +1003,7 @@ async fn push_index(
>       index: impl IndexFile + Send + 'static,
>       chunk_reader: Arc<dyn AsyncReadChunk>,
>       backup_writer: &BackupWriter,
> -    size: Option<u64>,
> +    index_type: IndexType,
>       known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
>   ) -> Result<SyncStats, Error> {
>       let (upload_channel_tx, upload_channel_rx) = mpsc::channel(20);
> @@ -1048,7 +1049,7 @@ async fn push_index(
>       let upload_options = UploadOptions {
>           compress: true,
>           encrypt: false,
> -        fixed_size: size,
> +        index_type,
>           ..UploadOptions::default()
>       };
>   

Re: [PATCH v5 proxmox-backup 10/16] client: treat minus sign as stdin

[Christian Ebner]

On 1/30/26 5:45 PM, Robert Obkircher wrote:
> Treat "-" as an alias for "/dev/stdin". If there is an actual file
> with that name it can still be read via "./-".
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   proxmox-backup-client/src/main.rs | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 7fc711fd..37878b01 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -845,6 +845,13 @@ async fn create_backup(
>           }
>           target_set.insert(target.clone());
>   
> +        // one can still use ./- to refer to an actual file with that name
> +        let filename = if filename == "-" {
> +            String::from("/dev/stdin")

should we add a warning here?

This is actually a breaking change, but guarding it behind a flag is 
maybe a bit overkill. We could check if the file exists before 
continuing and if so fail with an appropriate warning/error?

> +        } else {
> +            filename
> +        };
> +
>           use std::os::unix::fs::FileTypeExt;
>   
>           let metadata = std::fs::metadata(&filename)

Re: [PATCH v5 proxmox-backup 11/16] datastore: combine public FixedIndexWriter methods into add_chunk.

[Christian Ebner]

On 1/30/26 5:46 PM, Robert Obkircher wrote:
> These operations were always performed together anyway, so it makes
> sense to simplify the public api.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

as stated in the last review, IMO bundling the alignment check and 
add_digest() into add_chunk() should be done in a preparatory patch 
upfront, before introducing grow_to_size().

Re: [PATCH v5 proxmox-backup 12/16] datastore: use u64 instead of usize for fidx writer content size

[Christian Ebner]

On 1/30/26 5:46 PM, Robert Obkircher wrote:
> The file format is independent of the systems pointer size, so it
> makes sense to use a fixed size type.
> 
> The chunk size is also a u64 to avoid casts and because that is what
> is stored in the header anyway. Most other places limit it to u32, but
> widening shouldn't be an issue.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

Would like to hear other dev's opinions on this one and the latter 
patches of the series touching the size type. Not sure what the best way 
forward is here.

Re: [PATCH v5 proxmox-backup 07/16] api: backup: make fixed index file size optional

[Robert Obkircher]

On 2/2/26 12:38, Christian Ebner wrote:
> On 1/30/26 5:45 PM, Robert Obkircher wrote:
>> Pass the optional size to the FixedIndexWriter to make it grow as
>> necessary.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>> ---
>>   src/api2/backup/environment.rs | 21 ++++++++++++---------
>>   src/api2/backup/mod.rs         |  8 +++-----
>>   2 files changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/api2/backup/environment.rs
>> b/src/api2/backup/environment.rs
>> index bd9c5211..90a84ed2 100644
>> --- a/src/api2/backup/environment.rs
>> +++ b/src/api2/backup/environment.rs
>> @@ -67,7 +67,7 @@ struct DynamicWriterState {
>>   struct FixedWriterState {
>>       name: String,
>>       index: FixedIndexWriter,
>> -    size: usize,
>> +    size: Option<usize>,
>>       chunk_size: u32,
>>       chunk_count: u64,
>>       small_chunk_count: usize, // allow 0..1 small chunks (last
>> chunk may be smaller)
>> @@ -349,7 +349,7 @@ impl BackupEnvironment {
>>           &self,
>>           index: FixedIndexWriter,
>>           name: String,
>> -        size: usize,
>> +        size: Option<usize>,
>>           chunk_size: u32,
>>           incremental: bool,
>>       ) -> Result<usize, Error> {
>> @@ -443,6 +443,7 @@ impl BackupEnvironment {
>>           }
>>             let end = (offset as usize) + (size as usize);
>> +        data.index.grow_to_size(end)?;
>>           let idx = data.index.check_chunk_alignment(end, size as
>> usize)?;
>>             data.chunk_count += 1;
>> @@ -624,13 +625,15 @@ impl BackupEnvironment {
>>                   );
>>               }
>>   -            if size != (data.size as u64) {
>> -                bail!(
>> -                    "fixed writer '{}' close failed - unexpected
>> file size ({} != {})",
>> -                    data.name,
>> -                    data.size,
>> -                    size
>> -                );
>> +            if let Some(known_size) = data.size {
>> +                if size != known_size as u64 {
>> +                    bail!(
>> +                        "fixed writer '{}' close failed -
>> unexpected file size ({} != {})",
>> +                        data.name,
>> +                        known_size,
>> +                        size,
>> +                    );
>> +                }
>>               }
>>           }
>>   diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>> index c625a2dc..c2822c18 100644
>> --- a/src/api2/backup/mod.rs
>> +++ b/src/api2/backup/mod.rs
>> @@ -456,7 +456,7 @@ pub const API_METHOD_CREATE_FIXED_INDEX:
>> ApiMethod = ApiMethod::new(
>>               ("archive-name", false, &BACKUP_ARCHIVE_NAME_SCHEMA),
>>               (
>>                   "size",
>> -                false,
>> +                true,
>>                   &IntegerSchema::new("File
>> size.").minimum(1).schema()
>>               ),
>>               (
>> @@ -480,7 +480,7 @@ fn create_fixed_index(
>>       let env: &BackupEnvironment = rpcenv.as_ref();
>>         let name = required_string_param(&param,
>> "archive-name")?.to_owned();
>> -    let size = required_integer_param(&param, "size")? as usize;
>> +    let size =
>> param["size"].as_u64().map(usize::try_from).transpose()?;
>>       let reuse_csum = param["reuse-csum"].as_str();
>
> The fixed index writer does not allow for incremental backups,
> failing if it is requested.
>
> question: should we allow to proceed the backup without failing by
> simply switching to a non incremental backup for those index writers
> where no size is specified, including a log warning that that a
> non-incremental mode is used? 
I think reuse-csum without size could reasonably be supported by
copying the entire file and taking the size from there. Now that
add_chunk is a single operation it would even be possible to allow
overwriting the partial chunk in the end to continue growing.
>
> Should be better than failing the whole backup snapshot in case of
> multiple archives with mixed inputs.
As far as I can tell, "reuse-csum" is only set in one specific place
in proxmox-backup-qemu and only if the checksum matches exactly. Is
this actually relevant for anything else?
>
>>         let archive_name = name.clone();
>> @@ -528,9 +528,7 @@ fn create_fixed_index(
>>           reader = Some(index);
>>       }
>>   -    let mut writer = env
>> -        .datastore
>> -        .create_fixed_writer(&path, Some(size), chunk_size)?;
>> +    let mut writer = env.datastore.create_fixed_writer(&path,
>> size, chunk_size)?;
>>         if let Some(reader) = reader {
>>           writer.clone_data_from(&reader)?;
>

Re: [PATCH v5 proxmox-backup 07/16] api: backup: make fixed index file size optional

[Christian Ebner]

On 2/2/26 2:19 PM, Robert Obkircher wrote:
> 
> On 2/2/26 12:38, Christian Ebner wrote:
>> On 1/30/26 5:45 PM, Robert Obkircher wrote:
>>> Pass the optional size to the FixedIndexWriter to make it grow as
>>> necessary.
>>>
>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>> ---
>>>    src/api2/backup/environment.rs | 21 ++++++++++++---------
>>>    src/api2/backup/mod.rs         |  8 +++-----
>>>    2 files changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/api2/backup/environment.rs
>>> b/src/api2/backup/environment.rs
>>> index bd9c5211..90a84ed2 100644
>>> --- a/src/api2/backup/environment.rs
>>> +++ b/src/api2/backup/environment.rs
>>> @@ -67,7 +67,7 @@ struct DynamicWriterState {
>>>    struct FixedWriterState {
>>>        name: String,
>>>        index: FixedIndexWriter,
>>> -    size: usize,
>>> +    size: Option<usize>,
>>>        chunk_size: u32,
>>>        chunk_count: u64,
>>>        small_chunk_count: usize, // allow 0..1 small chunks (last
>>> chunk may be smaller)
>>> @@ -349,7 +349,7 @@ impl BackupEnvironment {
>>>            &self,
>>>            index: FixedIndexWriter,
>>>            name: String,
>>> -        size: usize,
>>> +        size: Option<usize>,
>>>            chunk_size: u32,
>>>            incremental: bool,
>>>        ) -> Result<usize, Error> {
>>> @@ -443,6 +443,7 @@ impl BackupEnvironment {
>>>            }
>>>              let end = (offset as usize) + (size as usize);
>>> +        data.index.grow_to_size(end)?;
>>>            let idx = data.index.check_chunk_alignment(end, size as
>>> usize)?;
>>>              data.chunk_count += 1;
>>> @@ -624,13 +625,15 @@ impl BackupEnvironment {
>>>                    );
>>>                }
>>>    -            if size != (data.size as u64) {
>>> -                bail!(
>>> -                    "fixed writer '{}' close failed - unexpected
>>> file size ({} != {})",
>>> -                    data.name,
>>> -                    data.size,
>>> -                    size
>>> -                );
>>> +            if let Some(known_size) = data.size {
>>> +                if size != known_size as u64 {
>>> +                    bail!(
>>> +                        "fixed writer '{}' close failed -
>>> unexpected file size ({} != {})",
>>> +                        data.name,
>>> +                        known_size,
>>> +                        size,
>>> +                    );
>>> +                }
>>>                }
>>>            }
>>>    diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>>> index c625a2dc..c2822c18 100644
>>> --- a/src/api2/backup/mod.rs
>>> +++ b/src/api2/backup/mod.rs
>>> @@ -456,7 +456,7 @@ pub const API_METHOD_CREATE_FIXED_INDEX:
>>> ApiMethod = ApiMethod::new(
>>>                ("archive-name", false, &BACKUP_ARCHIVE_NAME_SCHEMA),
>>>                (
>>>                    "size",
>>> -                false,
>>> +                true,
>>>                    &IntegerSchema::new("File
>>> size.").minimum(1).schema()
>>>                ),
>>>                (
>>> @@ -480,7 +480,7 @@ fn create_fixed_index(
>>>        let env: &BackupEnvironment = rpcenv.as_ref();
>>>          let name = required_string_param(&param,
>>> "archive-name")?.to_owned();
>>> -    let size = required_integer_param(&param, "size")? as usize;
>>> +    let size =
>>> param["size"].as_u64().map(usize::try_from).transpose()?;
>>>        let reuse_csum = param["reuse-csum"].as_str();
>>
>> The fixed index writer does not allow for incremental backups,
>> failing if it is requested.
>>
>> question: should we allow to proceed the backup without failing by
>> simply switching to a non incremental backup for those index writers
>> where no size is specified, including a log warning that that a
>> non-incremental mode is used?
> I think reuse-csum without size could reasonably be supported by
> copying the entire file and taking the size from there. Now that
> add_chunk is a single operation it would even be possible to allow
> overwriting the partial chunk in the end to continue growing.

Yes, if this can be supported as well would be great. It will however 
only allow to skip re-uploads, chunking of the stream on the client side 
has to happen anyways. And it must be assured that the stream might 
shrink, not just grow.

>>
>> Should be better than failing the whole backup snapshot in case of
>> multiple archives with mixed inputs.
> As far as I can tell, "reuse-csum" is only set in one specific place
> in proxmox-backup-qemu and only if the checksum matches exactly. Is
> this actually relevant for anything else?

No, AFAIK the checksum assures that the index cannot be reused for an 
incremental backup if e.g. there was a backup to the same backup group 
from a different client, which could lead to inconsistencies. So the 
checksum stored by the client must match the one found on the server to 
proceed safely, otherwise fallback to non-incremental.

>>
>>>          let archive_name = name.clone();
>>> @@ -528,9 +528,7 @@ fn create_fixed_index(
>>>            reader = Some(index);
>>>        }
>>>    -    let mut writer = env
>>> -        .datastore
>>> -        .create_fixed_writer(&path, Some(size), chunk_size)?;
>>> +    let mut writer = env.datastore.create_fixed_writer(&path,
>>> size, chunk_size)?;
>>>          if let Some(reader) = reader {
>>>            writer.clone_data_from(&reader)?;
>>