* [PATCH v6 proxmox-backup 01/18] datastore: remove Arc<ChunkStore> from FixedIndexWriter
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 02/18] datastore: remove Arc<ChunkStore> from DynamicIndexWriter Robert Obkircher
` (17 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 02/18] datastore: remove Arc<ChunkStore> from DynamicIndexWriter
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 01/18] datastore: remove Arc<ChunkStore> from FixedIndexWriter Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 03/18] datastore: add TempDir that is automatically deleted on drop Robert Obkircher
` (16 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
Remain consistent with the FixedIndexWriter and make it easier to add
tests in the future.
The ChunkStore reference can be safely removed, because it was only
needed for the base directory. As discussed previously [1], it is the
callers responsibility to ensure that the writer doesn't outlive any
locks.
[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 | 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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 03/18] datastore: add TempDir that is automatically deleted on drop
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 01/18] datastore: remove Arc<ChunkStore> from FixedIndexWriter Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 02/18] datastore: remove Arc<ChunkStore> from DynamicIndexWriter Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 04/18] datastore: use temporary directory for chunk store test Robert Obkircher
` (15 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
This will be used to test the FixedIndexWriter and chunk store
creation.
PDM has a similar type called NamedTempDir, so it would make sense to
move this to a shared crate in the future.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-datastore/src/lib.rs | 3 +
pbs-datastore/src/temp_dir.rs | 142 ++++++++++++++++++++++++++++++++++
2 files changed, 145 insertions(+)
create mode 100644 pbs-datastore/src/temp_dir.rs
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 1f7c54ae..f6c73bcb 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -233,3 +233,6 @@ pub use local_chunk_reader::LocalChunkReader;
mod local_datastore_lru_cache;
pub use local_datastore_lru_cache::LocalDatastoreLruCache;
+
+#[cfg(test)]
+mod temp_dir;
diff --git a/pbs-datastore/src/temp_dir.rs b/pbs-datastore/src/temp_dir.rs
new file mode 100644
index 00000000..bea97792
--- /dev/null
+++ b/pbs-datastore/src/temp_dir.rs
@@ -0,0 +1,142 @@
+use core::convert::AsRef;
+use std::convert::From;
+use std::env;
+use std::mem;
+use std::path::{Path, PathBuf};
+
+use anyhow::{Context, Error};
+
+/// A temporary directory that is automatically removed on drop.
+/// Deletion is best-effort and errors are silently ignored, unless
+/// the explicit `delete` method is used.
+#[derive(Debug)]
+pub struct TempDir {
+ directory: PathBuf,
+ delete_on_drop: bool,
+}
+
+impl TempDir {
+ /// Create an empty directory in `env::temp_dir()`.
+ pub fn new() -> Result<Self, Error> {
+ TempDir::new_in(env::temp_dir())
+ }
+
+ /// Create an empty child directory in the specified one.
+ pub fn new_in(parent: impl AsRef<Path>) -> Result<Self, Error> {
+ let parent = parent.as_ref();
+ let directory = proxmox_sys::fs::make_tmp_dir(parent, None)
+ .with_context(|| format!("Failed to create temporary directory in {parent:?}"))?;
+ Ok(Self {
+ directory,
+ delete_on_drop: true,
+ })
+ }
+
+ /// The path to this directory.
+ pub fn path(&self) -> &Path {
+ &self.directory
+ }
+
+ /// Disable automatic deletion.
+ /// May be useful when debugging tests.
+ pub fn disable_deletion(&mut self) {
+ self.delete_on_drop = false;
+ }
+
+ /// Disable atomatic deletion and turn into a normal `PathBuf`.
+ pub fn to_persistent(mut self) -> PathBuf {
+ self.disable_deletion();
+ mem::take(&mut self.directory)
+ }
+
+ /// Delete the directory with the possibility of handling the error.
+ pub fn delete(mut self) -> Result<(), Error> {
+ self.disable_deletion();
+ std::fs::remove_dir_all(&self.directory)
+ .with_context(|| format!("Failed to delete temporary directory {:?}", self.directory))
+ }
+}
+
+impl Drop for TempDir {
+ fn drop(&mut self) {
+ if self.delete_on_drop {
+ let _ = std::fs::remove_dir_all(&self.directory);
+ }
+ }
+}
+
+impl AsRef<Path> for TempDir {
+ fn as_ref(&self) -> &Path {
+ self.path()
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use std::fs;
+
+ #[test]
+ fn test_empty() {
+ let temp_dir = TempDir::new().unwrap();
+
+ let copy = PathBuf::from(temp_dir.path());
+ assert!(fs::exists(©).unwrap());
+ assert!(copy.is_dir());
+
+ drop(temp_dir);
+
+ assert!(!fs::exists(copy).unwrap());
+ }
+
+ #[test]
+ fn test_delete_with_content() {
+ let temp_dir = TempDir::new().unwrap();
+
+ let copy = PathBuf::from(temp_dir.path());
+ assert!(fs::exists(©).unwrap());
+ assert!(copy.is_dir());
+
+ fs::write(temp_dir.path().join("test.txt"), "hello").unwrap();
+
+ let subdir = temp_dir.path().join("foo").join("bar");
+ fs::create_dir_all(&subdir).unwrap();
+ fs::write(subdir.join("baz.txt"), "world").unwrap();
+
+ temp_dir.delete().unwrap();
+
+ assert!(!fs::exists(copy).unwrap());
+ }
+
+ #[test]
+ fn test_disable_deletion() {
+ let mut temp_dir = TempDir::new().unwrap();
+
+ let copy = PathBuf::from(temp_dir.path());
+ temp_dir.disable_deletion();
+ drop(temp_dir);
+
+ assert!(
+ fs::exists(©).unwrap(),
+ "Drop must not delte the directory"
+ );
+
+ fs::remove_dir_all(copy).unwrap(); // cleanup
+ }
+
+ #[test]
+ fn test_to_persistent() {
+ let temp_dir = TempDir::new().unwrap();
+
+ let copy = PathBuf::from(temp_dir.path());
+
+ let persistent = temp_dir.to_persistent();
+ assert_eq!(copy, persistent, "Path must be correct");
+ assert!(
+ fs::exists(&persistent).unwrap(),
+ "Directory must still exist."
+ );
+
+ fs::remove_dir_all(persistent).unwrap(); // cleanup
+ }
+}
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 04/18] datastore: use temporary directory for chunk store test
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (2 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 03/18] datastore: add TempDir that is automatically deleted on drop Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 05/18] datastore: combine public FixedIndexWriter methods into add_chunk Robert Obkircher
` (14 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index bd1dc353..1950be2b 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -980,19 +980,17 @@ impl ChunkExt {
#[test]
fn test_chunk_store1() {
- let mut path = std::fs::canonicalize(".").unwrap(); // we need absolute path
- path.push(".testdir");
+ let temp_dir = crate::temp_dir::TempDir::new().unwrap();
+ let path = temp_dir.path();
- if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
-
- let chunk_store = ChunkStore::open("test", &path, DatastoreFSyncLevel::None);
+ let chunk_store = ChunkStore::open("test", path, DatastoreFSyncLevel::None);
assert!(chunk_store.is_err());
let user = nix::unistd::User::from_uid(nix::unistd::Uid::current())
.unwrap()
.unwrap();
let chunk_store =
- ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
+ ChunkStore::create("test", path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
let (chunk, digest) = crate::data_blob::DataChunkBuilder::new(&[0u8, 1u8])
.build()
@@ -1005,8 +1003,8 @@ fn test_chunk_store1() {
assert!(exists);
let chunk_store =
- ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
+ 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 */ }
+ temp_dir.delete().unwrap();
}
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 05/18] datastore: combine public FixedIndexWriter methods into add_chunk
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (3 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 04/18] datastore: use temporary directory for chunk store test Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 06/18] datastore: use fixed size types for FixedIndexWriter Robert Obkircher
` (13 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 | 17 +++++++++++++++--
src/api2/backup/environment.rs | 5 +----
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index becbe3e3..081dc51f 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -351,7 +351,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);
}
@@ -381,7 +381,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 ({} >= {})",
@@ -403,6 +403,19 @@ 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.
+ 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}");
+ };
+ 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.index_length != reader.index_count() {
bail!("clone_data_from failed - index sizes not equal");
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index bd9c5211..517f698e 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -442,13 +442,10 @@ impl BackupEnvironment {
);
}
- let end = (offset as usize) + (size as usize);
- 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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 06/18] datastore: use fixed size types for FixedIndexWriter
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (4 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 05/18] datastore: combine public FixedIndexWriter methods into add_chunk Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 07/18] datastore: verify that chunk_size is a power of two Robert Obkircher
` (12 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
The file format is independent of the systems pointer size, so it is
better to use fixed size types.
For consistency with most other code, use u64 for the total content
size and u32 for the chunk size. Internally, the latter is also a u64
to avoid some casts and becasue that is what is stored in the header
anyway.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-datastore/src/datastore.rs | 4 ++--
pbs-datastore/src/fixed_index.rs | 33 +++++++++++++++++---------------
src/api2/backup/environment.rs | 6 +++---
src/api2/backup/mod.rs | 2 +-
4 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index b023b0d3..3f9c222d 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: usize,
- chunk_size: usize,
+ 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)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 081dc51f..7f0ef91c 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -214,8 +214,10 @@ pub struct FixedIndexWriter {
file: File,
filename: PathBuf,
tmp_filename: PathBuf,
- chunk_size: usize,
- size: usize,
+ /// Most places use u32 because values are just a few MiB, but here
+ /// u64 is sightly more convenient for calculations involving size.
+ chunk_size: u64,
+ size: u64,
index_length: usize,
index: *mut u8,
pub uuid: [u8; 16],
@@ -239,8 +241,8 @@ impl FixedIndexWriter {
// Requires obtaining a shared chunk store lock beforehand
pub fn create(
full_path: impl Into<PathBuf>,
- size: usize,
- chunk_size: usize,
+ size: u64,
+ chunk_size: u32,
) -> Result<Self, Error> {
let full_path = full_path.into();
let mut tmp_path = full_path.clone();
@@ -260,6 +262,8 @@ impl FixedIndexWriter {
panic!("got unexpected header size");
}
+ let chunk_size = u64::from(chunk_size);
+
let ctime = proxmox_time::epoch_i64();
let uuid = Uuid::generate();
@@ -269,15 +273,15 @@ impl FixedIndexWriter {
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.size = u64::to_le(size);
+ header.chunk_size = u64::to_le(chunk_size);
header.uuid = *uuid.as_bytes();
header.index_csum = [0u8; 32];
file.write_all(&buffer)?;
- let index_length = size.div_ceil(chunk_size);
+ let index_length = size.div_ceil(chunk_size).try_into()?;
let index_size = index_length * 32;
nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
@@ -351,12 +355,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);
@@ -378,7 +380,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> {
@@ -409,10 +411,11 @@ impl FixedIndexWriter {
/// content that is backed up. It is verified that `start` is
/// aligned and that only the last chunk may be smaller.
pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
- let Some(end) = start.checked_add(size.into()) else {
+ let size = u64::from(size);
+ let Some(end) = start.checked_add(size) else {
bail!("add_chunk: start and size are too large: {start}+{size}");
};
- let idx = self.check_chunk_alignment(end as usize, size as usize)?;
+ let idx = self.check_chunk_alignment(end, size)?;
self.add_digest(idx, digest)
}
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 517f698e..234b2582 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: 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: usize,
+ size: u64,
chunk_size: u32,
incremental: bool,
) -> Result<usize, Error> {
@@ -621,7 +621,7 @@ impl BackupEnvironment {
);
}
- if size != (data.size as u64) {
+ if size != data.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 3e6b7a95..6e3b46c2 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(¶m, "archive-name")?.to_owned();
- let size = required_integer_param(¶m, "size")? as usize;
+ let size = required_integer_param(¶m, "size")? as u64;
let reuse_csum = param["reuse-csum"].as_str();
let archive_name = name.clone();
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 07/18] datastore: verify that chunk_size is a power of two
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (5 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 06/18] datastore: use fixed size types for FixedIndexWriter Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-17 9:13 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 08/18] datastore: support writing fidx files of unknown size Robert Obkircher
` (11 subsequent siblings)
18 siblings, 1 reply; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
Guarantee that the bitmasks used to compute the modulo actually yield
the correct value and implicitly guard against divisions by zero.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-datastore/src/fixed_index.rs | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 7f0ef91c..953f4a20 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -86,6 +86,10 @@ impl FixedIndexReader {
let ctime = i64::from_le(header.ctime);
let chunk_size = u64::from_le(header.chunk_size);
+ if !chunk_size.is_power_of_two() {
+ bail!("got non-power-of-two chunk size: {chunk_size}");
+ }
+
let index_length = size.div_ceil(chunk_size) as usize;
let index_size = index_length * 32;
@@ -98,6 +102,8 @@ impl FixedIndexReader {
);
}
+ let chunk_size = usize::try_from(chunk_size)?;
+
let data = unsafe {
nix::sys::mman::mmap(
None,
@@ -114,7 +120,7 @@ impl FixedIndexReader {
Ok(Self {
_file: file,
- chunk_size: chunk_size as usize,
+ chunk_size,
size,
index_length,
index: data,
@@ -263,6 +269,9 @@ impl FixedIndexWriter {
}
let chunk_size = u64::from(chunk_size);
+ if !chunk_size.is_power_of_two() {
+ bail!("got non-power-of-two chunk size: {chunk_size}");
+ }
let ctime = proxmox_time::epoch_i64();
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 08/18] datastore: support writing fidx files of unknown size
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (6 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 07/18] datastore: verify that chunk_size is a power of two Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 09/18] datastore: test FixedIndexWriter Robert Obkircher
` (10 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 | 135 ++++++++++++++++++++++++++++++-
src/api2/backup/mod.rs | 6 +-
3 files changed, 136 insertions(+), 7 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 3f9c222d..631bdc30 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: u64,
+ size: Option<u64>,
chunk_size: u32,
) -> 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 953f4a20..3807ba35 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -225,9 +225,12 @@ pub struct FixedIndexWriter {
chunk_size: u64,
size: u64,
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
@@ -243,11 +246,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: u64,
+ known_size: Option<u64>,
chunk_size: u32,
) -> Result<Self, Error> {
let full_path = full_path.into();
@@ -274,6 +287,7 @@ impl FixedIndexWriter {
}
let ctime = proxmox_time::epoch_i64();
+ let size = known_size.unwrap_or(0);
let uuid = Uuid::generate();
@@ -290,8 +304,15 @@ impl FixedIndexWriter {
file.write_all(&buffer)?;
- let index_length = size.div_ceil(chunk_size).try_into()?;
- let index_size = index_length * 32;
+ 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)?;
let data = unsafe {
@@ -315,12 +336,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: 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).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)?;
+ } 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
}
@@ -330,7 +429,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);
@@ -352,9 +451,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.to_le_bytes())?;
+ }
+
self.file.flush()?;
if let Err(err) = std::fs::rename(&self.tmp_filename, &self.filename) {
@@ -419,16 +533,29 @@ impl FixedIndexWriter {
/// 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 size = u64::from(size);
let Some(end) = start.checked_add(size) 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)?;
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");
+ }
+
+ if self.chunk_size != reader.chunk_size as u64 {
+ 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 6e3b46c2..b8f34dfa 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -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, Some(size), chunk_size)?;
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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 09/18] datastore: test FixedIndexWriter
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (7 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 08/18] datastore: support writing fidx files of unknown size Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 10/18] api: backup: make fixed index file size optional Robert Obkircher
` (9 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 | 178 +++++++++++++++++++++++++++++++
1 file changed, 178 insertions(+)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 3807ba35..350bf2ed 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -567,3 +567,181 @@ impl FixedIndexWriter {
Ok(())
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::fs;
+
+ use super::*;
+ use crate::temp_dir::TempDir;
+
+ const CS: u32 = 4096;
+
+ #[test]
+ fn test_empty() {
+ let dir = TempDir::new().unwrap();
+ let path = dir.path().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());
+
+ dir.delete().unwrap();
+ }
+
+ #[test]
+ fn test_single_partial_chunk() {
+ let dir = TempDir::new().unwrap();
+ let path = dir.path().join("test_single_partial_chunk");
+ let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+ let size = CS as u64 - 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);
+
+ dir.delete().unwrap();
+ }
+
+ #[test]
+ fn test_grow_to_multiples_of_chunk_size() {
+ let dir = TempDir::new().unwrap();
+ let path = dir.path().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() as u64 * CS as u64);
+
+ 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() as u64 * CS as u64;
+ check_with_reader(&path, size, &expected);
+ compare_to_known_size_writer(&path, size, &expected);
+
+ dir.delete().unwrap();
+ }
+
+ #[test]
+ fn test_grow_to_misaligned_size() {
+ let dir = TempDir::new().unwrap();
+ let path = dir.path().join("test_grow_to_misaligned_size");
+ let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+ 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();
+ 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);
+
+ dir.delete().unwrap();
+ }
+
+ struct TestChunk {
+ digest: [u8; 32],
+ index: usize,
+ size: u32,
+ end: u64,
+ }
+
+ impl TestChunk {
+ fn add_to(&self, w: &mut FixedIndexWriter) {
+ assert_eq!(
+ self.index,
+ w.check_chunk_alignment(self.end, self.size as u64).unwrap()
+ );
+ w.add_digest(self.index, &self.digest).unwrap();
+ }
+ }
+
+ fn test_data(size: u64) -> Vec<TestChunk> {
+ (0..size.div_ceil(CS as u64))
+ .map(|index| {
+ let mut digest = [0u8; 32];
+ let i = &index.to_le_bytes();
+ for c in digest.chunks_mut(i.len()) {
+ c.copy_from_slice(i);
+ }
+ let size = if ((index + 1) * CS as u64) <= size {
+ CS
+ } else {
+ (size % CS as u64) as u32
+ };
+ TestChunk {
+ digest,
+ index: index as usize,
+ size,
+ end: index * CS as u64 + size as u64,
+ }
+ })
+ .collect()
+ }
+
+ fn check_with_reader(path: &Path, size: u64, chunks: &[TestChunk]) {
+ let reader = FixedIndexReader::open(path).unwrap();
+ 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: u64, 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);
+ }
+}
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 10/18] api: backup: make fixed index file size optional
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (8 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 09/18] datastore: test FixedIndexWriter Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 11/18] api: verify fixed index writer size on close Robert Obkircher
` (8 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 | 20 +++++++++++---------
src/api2/backup/mod.rs | 8 +++-----
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 234b2582..39fe0564 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: u64,
+ 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: u64,
+ size: Option<u64>,
chunk_size: u32,
incremental: bool,
) -> Result<usize, Error> {
@@ -621,13 +621,15 @@ impl BackupEnvironment {
);
}
- if size != data.size {
- bail!(
- "fixed writer '{}' close failed - unexpected file size ({} != {})",
- data.name,
- data.size,
- size
- );
+ if let Some(known_size) = data.size {
+ if size != known_size {
+ 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 b8f34dfa..4d0846ca 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(¶m, "archive-name")?.to_owned();
- let size = required_integer_param(¶m, "size")? as u64;
+ let size = param["size"].as_u64();
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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 11/18] api: verify fixed index writer size on close
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (9 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 10/18] api: backup: make fixed index file size optional Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 12/18] client: don't poll terminated source in FixedChunkStream Robert Obkircher
` (7 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
Compare it to the value submitted by the client to check that 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 350bf2ed..e6d6f3df 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -424,6 +424,11 @@ impl FixedIndexWriter {
self.index_length
}
+ /// The current total size of the referenced content.
+ pub fn size(&self) -> u64 {
+ self.size
+ }
+
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 39fe0564..9645f6de 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -633,6 +633,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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 12/18] client: don't poll terminated source in FixedChunkStream
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (10 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 11/18] api: verify fixed index writer size on close Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-17 10:01 ` Christian Ebner
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 13/18] client: don't poll terminated source in ChunkStream Robert Obkircher
` (6 subsequent siblings)
18 siblings, 1 reply; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
Fix incorrect chunking when the input file is a pts character device
(stdin in an interactive terminal). Previously, the stream could
produce multiple smaller chunks from interactive input, and a single
Ctrl+D was not enough to terminate it. This was because a None from
the source resulted in Some partial chunk, but the next time the input
was polled again and it suddenly had more input.
The documentation of Stream::poll_next clearly states that calling it
on a terminated stream may panic, block forever, or cause other kinds
of problems, so this should be fixed even it it appeared to work with
normal files.
Using the fuse method or adding a FusedStream trait bound might be
nicer that seemed like a larger change compared to the boolean.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-client/src/chunk_stream.rs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/pbs-client/src/chunk_stream.rs b/pbs-client/src/chunk_stream.rs
index e3f0980c..94a45ac7 100644
--- a/pbs-client/src/chunk_stream.rs
+++ b/pbs-client/src/chunk_stream.rs
@@ -187,6 +187,7 @@ pub struct FixedChunkStream<S: Unpin> {
input: S,
chunk_size: usize,
buffer: BytesMut,
+ done: bool,
}
impl<S: Unpin> FixedChunkStream<S> {
@@ -195,6 +196,7 @@ impl<S: Unpin> FixedChunkStream<S> {
input,
chunk_size,
buffer: BytesMut::new(),
+ done: false,
}
}
}
@@ -213,6 +215,9 @@ where
cx: &mut Context,
) -> Poll<Option<Result<BytesMut, S::Error>>> {
let this = self.get_mut();
+ if this.done {
+ return Poll::Ready(None);
+ }
loop {
if this.buffer.len() >= this.chunk_size {
return Poll::Ready(Some(Ok(this.buffer.split_to(this.chunk_size))));
@@ -223,6 +228,9 @@ where
return Poll::Ready(Some(Err(err)));
}
None => {
+ // Must not call input.try_poll_next again!
+ this.done = true;
+
// last chunk can have any size
if !this.buffer.is_empty() {
return Poll::Ready(Some(Ok(this.buffer.split())));
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v6 proxmox-backup 12/18] client: don't poll terminated source in FixedChunkStream
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 12/18] client: don't poll terminated source in FixedChunkStream Robert Obkircher
@ 2026-02-17 10:01 ` Christian Ebner
2026-02-17 10:06 ` Christian Ebner
0 siblings, 1 reply; 24+ messages in thread
From: Christian Ebner @ 2026-02-17 10:01 UTC (permalink / raw)
To: Robert Obkircher, pbs-devel
On 2/10/26 4:06 PM, Robert Obkircher wrote:
> Fix incorrect chunking when the input file is a pts character device
> (stdin in an interactive terminal). Previously, the stream could
> produce multiple smaller chunks from interactive input, and a single
> Ctrl+D was not enough to terminate it. This was because a None from
> the source resulted in Some partial chunk, but the next time the input
> was polled again and it suddenly had more input.
>
> The documentation of Stream::poll_next clearly states that calling it
> on a terminated stream may panic, block forever, or cause other kinds
> of problems, so this should be fixed even it it appeared to work with
> normal files.
>
> Using the fuse method or adding a FusedStream trait bound might be
> nicer that seemed like a larger change compared to the boolean.
>
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
Good catch! Same issue is however also present for the dynamic
ChunkStream, so the safeguard should be added for it as well!
That can however be done as followup patch if nothing else comes up for
these patches.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v6 proxmox-backup 12/18] client: don't poll terminated source in FixedChunkStream
2026-02-17 10:01 ` Christian Ebner
@ 2026-02-17 10:06 ` Christian Ebner
0 siblings, 0 replies; 24+ messages in thread
From: Christian Ebner @ 2026-02-17 10:06 UTC (permalink / raw)
To: Robert Obkircher, pbs-devel
On 2/17/26 11:01 AM, Christian Ebner wrote:
> On 2/10/26 4:06 PM, Robert Obkircher wrote:
>> Fix incorrect chunking when the input file is a pts character device
>> (stdin in an interactive terminal). Previously, the stream could
>> produce multiple smaller chunks from interactive input, and a single
>> Ctrl+D was not enough to terminate it. This was because a None from
>> the source resulted in Some partial chunk, but the next time the input
>> was polled again and it suddenly had more input.
>>
>> The documentation of Stream::poll_next clearly states that calling it
>> on a terminated stream may panic, block forever, or cause other kinds
>> of problems, so this should be fixed even it it appeared to work with
>> normal files.
>>
>> Using the fuse method or adding a FusedStream trait bound might be
>> nicer that seemed like a larger change compared to the boolean.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>
> Good catch! Same issue is however also present for the dynamic
> ChunkStream, so the safeguard should be added for it as well!
>
> That can however be done as followup patch if nothing else comes up for
> these patches.
Disregard this comment, did not see the next patch, which already does this!
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v6 proxmox-backup 13/18] client: don't poll terminated source in ChunkStream
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (11 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 12/18] client: don't poll terminated source in FixedChunkStream Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 14/18] fix #3847: client: support fifo pipe inputs for image backups Robert Obkircher
` (5 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
The ChunkStream may return a partial chunk after the input is done.
This is fine, but it must remember to not poll the input again when it
is polled one more time.
The DummyInput of the test case now verifies that it isn't polled
after it returned None.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-client/src/chunk_stream.rs | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/pbs-client/src/chunk_stream.rs b/pbs-client/src/chunk_stream.rs
index 94a45ac7..9763bbd7 100644
--- a/pbs-client/src/chunk_stream.rs
+++ b/pbs-client/src/chunk_stream.rs
@@ -34,6 +34,7 @@ impl InjectionData {
/// Split input stream into dynamic sized chunks
pub struct ChunkStream<S: Unpin> {
input: S,
+ input_is_done: bool,
chunker: Box<dyn Chunker + Send>,
buffer: BytesMut,
scan_pos: usize,
@@ -51,6 +52,7 @@ impl<S: Unpin> ChunkStream<S> {
let chunk_size = chunk_size.unwrap_or(4 * 1024 * 1024);
Self {
input,
+ input_is_done: false,
chunker: if let Some(suggested) = suggested_boundaries {
Box::new(PayloadChunker::new(chunk_size, suggested))
} else {
@@ -162,12 +164,16 @@ where
}
}
+ if this.input_is_done {
+ return Poll::Ready(None);
+ }
match ready!(Pin::new(&mut this.input).try_poll_next(cx)) {
Some(Err(err)) => {
return Poll::Ready(Some(Err(err.into())));
}
None => {
this.scan_pos = 0;
+ this.input_is_done = true;
if !this.buffer.is_empty() {
return Poll::Ready(Some(Ok(this.buffer.split())));
} else {
@@ -254,11 +260,12 @@ mod test {
struct DummyInput {
data: Vec<u8>,
+ done: bool,
}
impl DummyInput {
fn new(data: Vec<u8>) -> Self {
- Self { data }
+ Self { data, done: false }
}
}
@@ -268,7 +275,11 @@ mod test {
fn poll_next(self: Pin<&mut Self>, _cx: &mut Context) -> Poll<Option<Self::Item>> {
let this = self.get_mut();
match this.data.len() {
- 0 => Poll::Ready(None),
+ 0 => {
+ assert!(!this.done);
+ this.done = true;
+ Poll::Ready(None)
+ }
size if size > 10 => Poll::Ready(Some(Ok(this.data.split_off(10)))),
_ => Poll::Ready(Some(Ok(std::mem::take(&mut this.data)))),
}
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 14/18] fix #3847: client: support fifo pipe inputs for image backups
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (12 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 13/18] client: don't poll terminated source in ChunkStream Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 15/18] client: Fail early if the same pipe is specified for multiple inputs Robert Obkircher
` (4 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 | 37 +++++++++++++++++--------
proxmox-backup-client/src/main.rs | 46 +++++++++++++++++++++----------
src/server/push.rs | 11 ++++----
3 files changed, 63 insertions(+), 31 deletions(-)
diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index dbd177d8..49aff3fd 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -52,7 +52,26 @@ 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>),
+}
+
+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 {
@@ -292,12 +311,10 @@ impl BackupWriter {
options: UploadOptions,
) -> Result<BackupStats, Error> {
let mut param = json!({ "archive-name": archive_name });
- let prefix = if let Some(size) = options.fixed_size {
+ let (prefix, archive_size) = options.index_type.to_prefix_and_size();
+ if let Some(size) = archive_size {
param["size"] = size.into();
- "fixed"
- } else {
- "dynamic"
- };
+ }
if options.encrypt && self.crypt_config.is_none() {
bail!("requested encryption without a crypt config");
@@ -387,12 +404,10 @@ 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 {
+ let (prefix, archive_size) = options.index_type.to_prefix_and_size();
+ if let Some(size) = archive_size {
param["size"] = size.into();
- "fixed"
- } else {
- "dynamic"
- };
+ }
if options.encrypt && self.crypt_config.is_none() {
bail!("requested encryption without a crypt config");
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 999e5020..1a51e88f 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!");
}
@@ -847,8 +847,12 @@ async fn create_backup(
use std::os::unix::fs::FileTypeExt;
- let metadata = std::fs::metadata(&filename)
- .map_err(|err| format_err!("unable to access '{}' - {}", filename, err))?;
+ let metadata = std::fs::metadata(&filename).map_err(|err| {
+ if filename == "-" {
+ log::info!("If you are trying to read from stdin use '/dev/stdin' instead of '-'.");
+ }
+ format_err!("unable to access '{}' - {}", filename, err)
+ })?;
let file_type = metadata.file_type();
match spec_type {
@@ -859,15 +863,25 @@ 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 {
+ if file_type.is_char_device() {
+ // This would technically work, but allowing interactive
+ // inputs is error prone, and devices like /dev/random
+ // are often infinite streams.
+ log::info!(
+ "Character devices (including pseudo terminals) are not supported."
+ );
+ }
+ bail!("got unexpected file type (expected file, block device, or fifo): {filename}");
+ };
upload_list.push((
BackupSpecificationType::IMAGE,
@@ -1191,9 +1205,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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 15/18] client: Fail early if the same pipe is specified for multiple inputs
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (13 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 14/18] fix #3847: client: support fifo pipe inputs for image backups Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 16/18] datastore: compute fidx file size with overflow checks Robert Obkircher
` (3 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
The pipe needs to be closed to finish the first archive, so the second
one would always end up being empty.
Duplicates are detected using the device id and inode number of the
metadata. The inode number alone would not be sufficient for named
pipes in a specific file system.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
proxmox-backup-client/src/main.rs | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 1a51e88f..dcf7f84d 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -832,6 +832,7 @@ async fn create_backup(
let mut upload_list = vec![];
let mut target_set = HashSet::new();
+ let mut pipe_set = HashSet::new();
for backupspec in backupspec_list {
let pbs_client::BackupSpecification {
@@ -845,7 +846,7 @@ async fn create_backup(
}
target_set.insert(target.clone());
- use std::os::unix::fs::FileTypeExt;
+ use std::os::unix::fs::{FileTypeExt, MetadataExt};
let metadata = std::fs::metadata(&filename).map_err(|err| {
if filename == "-" {
@@ -870,6 +871,11 @@ async fn create_backup(
}
size
} else if file_type.is_fifo() {
+ // dev is needed for named pipes
+ if !pipe_set.insert((metadata.dev(), metadata.ino())) {
+ // The second archive would be empty
+ bail!("got duplicate pipe '{filename}'");
+ }
0
} else {
if file_type.is_char_device() {
@@ -917,6 +923,7 @@ async fn create_backup(
}
}
}
+ drop(pipe_set);
let backup_time = backup_time_opt.unwrap_or_else(epoch_i64);
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 16/18] datastore: compute fidx file size with overflow checks
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (14 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 15/18] client: Fail early if the same pipe is specified for multiple inputs Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 17/18] datastore: support writing fidx files on systems with larger page size Robert Obkircher
` (2 subsequent siblings)
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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 e6d6f3df..372d4d91 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -313,7 +313,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(
@@ -345,6 +346,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> {
@@ -353,7 +370,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.")
@@ -456,11 +473,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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 17/18] datastore: support writing fidx files on systems with larger page size
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (15 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 16/18] datastore: compute fidx file size with overflow checks Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 18/18] datastore: support incremental fidx uploads with different size Robert Obkircher
2026-02-17 12:42 ` [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Christian Ebner
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
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.
This also removes a cast from a potentially insufficiently aligned u8
buffer to the header, which could have caused undefined behavior.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-datastore/src/fixed_index.rs | 122 ++++++++++++++-----------------
1 file changed, 54 insertions(+), 68 deletions(-)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 372d4d91..ccbae72b 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};
@@ -216,6 +215,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,
@@ -226,11 +237,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
@@ -256,7 +266,6 @@ impl FixedIndexWriter {
/// 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>,
@@ -267,7 +276,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)
@@ -291,19 +300,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);
- header.chunk_size = u64::to_le(chunk_size);
- 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()?;
@@ -312,23 +308,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,
@@ -338,11 +336,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(),
})
}
@@ -368,27 +365,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(())
}
@@ -401,7 +397,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)
})
}
@@ -447,30 +443,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 {
@@ -479,18 +477,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.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);
}
@@ -535,13 +521,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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v6 proxmox-backup 18/18] datastore: support incremental fidx uploads with different size
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (16 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 17/18] datastore: support writing fidx files on systems with larger page size Robert Obkircher
@ 2026-02-10 15:06 ` Robert Obkircher
2026-02-17 12:42 ` [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Christian Ebner
18 siblings, 0 replies; 24+ messages in thread
From: Robert Obkircher @ 2026-02-10 15:06 UTC (permalink / raw)
To: pbs-devel
Copy as much as possible instead of requiring the index lengths to
match exactly. For resizable writers the capacity is increased
beforehand, but size and index length are only updated when chunks
are added or with the final size on close.
The partial chunk in the end is not tracked. It is the clients
responsibility to overwrite it if necessary.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
pbs-datastore/src/fixed_index.rs | 66 +++++++++++++++++++++++++++-----
src/api2/backup/environment.rs | 4 ++
2 files changed, 61 insertions(+), 9 deletions(-)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index ccbae72b..33a3734b 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -520,7 +520,10 @@ impl FixedIndexWriter {
self.index_length
);
}
+ self.add_digest_unchecked(index, digest)
+ }
+ fn add_digest_unchecked(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
let Some(ptr) = &self.memory else {
bail!("cannot write to closed index file.");
};
@@ -553,23 +556,23 @@ impl FixedIndexWriter {
self.add_digest(idx, digest)
}
+ /// Copy the chunk hashes from a Reader to the start of this Writer.
+ ///
+ /// If this writer is resizable the capacity may increase,
+ /// but the size and length stay the same.
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 as u64 {
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");
+ let count = reader.index_count();
+ if self.growable_size && self.index_capacity < count {
+ self.set_index_capacity_or_unmap(count)?;
}
- for i in 0..self.index_length {
- self.add_digest(i, reader.index_digest(i).unwrap())?;
+ for i in 0..count.min(self.index_capacity) {
+ self.add_digest_unchecked(i, reader.index_digest(i).unwrap())?;
}
-
Ok(())
}
}
@@ -682,6 +685,51 @@ mod tests {
dir.delete().unwrap();
}
+ #[test]
+ fn test_clone_data_from() {
+ let dir = TempDir::new().unwrap();
+ let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 3) * CS as u64;
+ let mut expected = test_data(size);
+
+ let reused = dir.path().join("reused");
+ let mut w = FixedIndexWriter::create(&reused, Some(size), CS).unwrap();
+ for c in expected.iter() {
+ c.add_to(&mut w);
+ }
+ w.close().unwrap();
+ drop(w);
+
+ let reused = FixedIndexReader::open(&reused).unwrap();
+
+ let truncated = dir.path().join("truncated");
+ let size = size - CS as u64;
+ expected.pop();
+ let mut w = FixedIndexWriter::create(&truncated, Some(size), CS).unwrap();
+ w.clone_data_from(&reused).unwrap();
+ w.close().unwrap();
+ drop(w);
+ check_with_reader(&truncated, size, &expected);
+ compare_to_known_size_writer(&truncated, size, &expected);
+
+ let modified = dir.path().join("modified");
+ let mut w = FixedIndexWriter::create(&modified, None, CS).unwrap();
+ w.clone_data_from(&reused).unwrap();
+ {
+ let i = expected.len() / 2;
+ expected[i].digest[1] += 1;
+ let chunk = &expected[i];
+ let chunk_pos = chunk.end - chunk.size as u64;
+ w.add_chunk(chunk_pos, chunk.size, &chunk.digest).unwrap();
+ }
+ w.grow_to_size(size).unwrap();
+ w.close().unwrap();
+ drop(w);
+ check_with_reader(&modified, size, &expected);
+ compare_to_known_size_writer(&modified, size, &expected);
+
+ dir.delete().unwrap();
+ }
+
struct TestChunk {
digest: [u8; 32],
index: usize,
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 9645f6de..7063a706 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -609,6 +609,10 @@ impl BackupEnvironment {
);
}
+ if data.incremental && data.size.is_none() {
+ data.index.grow_to_size(size)?;
+ }
+
if !data.incremental {
let expected_count = data.index.index_length();
--
2.47.3
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client
2026-02-10 15:06 [PATCH v6 proxmox-backup 00/18] fix: #3847 pipe from STDIN to proxmox-backup-client Robert Obkircher
` (17 preceding siblings ...)
2026-02-10 15:06 ` [PATCH v6 proxmox-backup 18/18] datastore: support incremental fidx uploads with different size Robert Obkircher
@ 2026-02-17 12:42 ` Christian Ebner
18 siblings, 0 replies; 24+ messages in thread
From: Christian Ebner @ 2026-02-17 12:42 UTC (permalink / raw)
To: Robert Obkircher, pbs-devel
On 2/10/26 4:06 PM, Robert Obkircher wrote:
> 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 v5:
>
> Client:
> - add IndexType::to_prefix_and_size helper and fix format string nit
> - drop support for '-' but log helpful warnings
> - fail early for duplicate pipes
> - avoid calling poll_next on terminated streams
>
> Server:
> - mention why Arc<ChunkStore> can be removed from didx as well
> - (Test)TempDir:
> - add to parent mod in earlier commit
> - drop "Test" from the name
> - improved API so it could be moved to a crate (no unwrap in new,
> no Deref, no impl From<&TempDir> for PathBuf)
> - added tests
> - reorder add_chunk commit before grow_to_size
> - support reuse-csum with arbitrary sizes
> - made up my mind on u32 vs u64 and reordered commits
> - check that chunk_size is power of two
This series has taken a really nice shape, got nothing to complain code
wise and works as expected!
Tested:
- several input stream variations, via pipe and command substitution
- restores produce the same byte stream as the input data
- char devices and pts are not allowed
- reading from /dev/stdin is not allowed to be duplicated
- subsequent backups work without issues
- backup and restore of images with given size such as for VMs are
unaffected
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
^ permalink raw reply [flat|nested] 24+ messages in thread