* [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool
@ 2024-03-05 13:56 Filip Schauer
2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit Filip Schauer
` (7 more replies)
0 siblings, 8 replies; 14+ messages in thread
From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw)
To: pbs-devel
Implement a tool to import VMA files into a Proxmox Backup Server
Example usage:
zstd -d --stdout vzdump.vma.zstd | vma-to-pbs \
--repository <auth_id@host:port:datastore> \
--vmid 123 \
--password_file pbs_password
Changes since v3:
* Add the ability to provide credentials via files
* Add support for streaming the VMA file via stdin
* Add a fallback for the --fingerprint argument
Changes since v2:
* Use the deb packages from the proxmox-io and proxmox-sys dependencies
instead of the proxmox submodule
* Remove the proxmox submodule
* Update the proxmox-backup-qemu submodule to make it buildable with
the newest librust dependencies
Changes since v1:
* Remove unused crates and uses
* Format the code
* Use anyhow for error handling
* Use clap for parsing arguments instead of getopts
* Fix blocks being reindexed on every read
* Make sure ProxmoxBackupHandle is dropped properly on error
* Move image_chunk_buffer from stack to heap
* Move the block_index in VmaReader to the heap completely
* Initialize vectors with `Vec::with_capacity` and `resize` instead of
the `vec!` macro, to potentially improve performance on debug builds.
* Add comments to code filling the MD5 sum field with zeros
* Change device_id arguments to usize
* Handle devices that have a size that is not aligned to 4096 properly
in read_device_contents, when the caller provides a buffer that would
exceed the device size.
* Avoid unnecessary loop iterations in read_device_contents when the
buffer size is not aligned to 65536
Filip Schauer (4):
Initial commit
Add the ability to provide credentials via files
Add support for streaming the VMA file via stdin
Add a fallback for the --fingerprint argument
Wolfgang Bumiller (2):
cargo fmt
bump proxmox-backup-qemu
--
2.39.2
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer @ 2024-03-05 13:56 ` Filip Schauer 2024-03-06 15:49 ` Max Carrara 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 2/6] cargo fmt Filip Schauer ` (6 subsequent siblings) 7 siblings, 1 reply; 14+ messages in thread From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw) To: pbs-devel Implement a tool to import VMA files into a Proxmox Backup Server Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- .cargo/config | 5 + .gitmodules | 3 + Cargo.toml | 19 ++ Makefile | 70 +++++++ src/main.rs | 311 ++++++++++++++++++++++++++++++ src/vma.rs | 340 +++++++++++++++++++++++++++++++++ submodules/proxmox-backup-qemu | 1 + 7 files changed, 749 insertions(+) create mode 100644 .cargo/config create mode 100644 .gitmodules create mode 100644 Cargo.toml create mode 100644 Makefile create mode 100644 src/main.rs create mode 100644 src/vma.rs create mode 160000 submodules/proxmox-backup-qemu diff --git a/.cargo/config b/.cargo/config new file mode 100644 index 0000000..3b5b6e4 --- /dev/null +++ b/.cargo/config @@ -0,0 +1,5 @@ +[source] +[source.debian-packages] +directory = "/usr/share/cargo/registry" +[source.crates-io] +replace-with = "debian-packages" diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 0000000..6d21e06 --- /dev/null +++ b/.gitmodules @@ -0,0 +1,3 @@ +[submodule "submodules/proxmox-backup-qemu"] + path = submodules/proxmox-backup-qemu + url = git://git.proxmox.com/git/proxmox-backup-qemu.git diff --git a/Cargo.toml b/Cargo.toml new file mode 100644 index 0000000..9711690 --- /dev/null +++ b/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "vma-to-pbs" +version = "0.0.1" +authors = ["Filip Schauer <f.schauer@proxmox.com>"] +edition = "2021" + +[dependencies] +anyhow = "1.0" +bincode = "1.3" +clap = { version = "4.0.32", features = ["cargo"] } +md5 = "0.7.0" +scopeguard = "1.1.0" +serde = "1.0" +serde-big-array = "0.4.1" + +proxmox-io = "1.0.1" +proxmox-sys = "0.5.0" + +proxmox-backup-qemu = { path = "submodules/proxmox-backup-qemu" } diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..a0c841d --- /dev/null +++ b/Makefile @@ -0,0 +1,70 @@ +include /usr/share/dpkg/default.mk + +PACKAGE = proxmox-vma-to-pbs +BUILDDIR = $(PACKAGE)-$(DEB_VERSION_UPSTREAM) + +ARCH := $(DEB_BUILD_ARCH) + +DSC=$(DEB_SOURCE)_$(DEB_VERSION).dsc +MAIN_DEB=$(PACKAGE)_$(DEB_VERSION)_$(ARCH).deb +OTHER_DEBS = \ + $(PACKAGE)-dev_$(DEB_VERSION)_$(ARCH).deb \ + $(PACKAGE)-dbgsym_$(DEB_VERSION)_$(ARCH).deb +DEBS=$(MAIN_DEB) $(OTHER_DEBS) + +DESTDIR= + +TARGET_DIR := target/debug + +ifeq ($(BUILD_MODE), release) +CARGO_BUILD_ARGS += --release +TARGETDIR := target/release +endif + +.PHONY: all build +all: build + +build: $(TARGETDIR)/vma-to-pbs +$(TARGETDIR)/vma-to-pbs: Cargo.toml src/ + cargo build $(CARGO_BUILD_ARGS) + +.PHONY: install +install: $(TARGETDIR)/vma-to-pbs + install -D -m 0755 $(TARGETDIR)/vma-to-pbs $(DESTDIR)/usr/bin/vma-to-pbs + +$(BUILDDIR): submodule + rm -rf $@ $@.tmp && mkdir $@.tmp + cp -a submodules debian Makefile .cargo Cargo.toml build.rs src $@.tmp/ + mv $@.tmp $@ + +submodule: + [ -e submodules/proxmox-backup-qemu/Cargo.toml ] || [ -e submodules/proxmox/proxmox-sys/Cargo.toml ] || git submodule update --init --recursive + +dsc: + rm -rf $(BUILDDIR) $(DSC) + $(MAKE) $(DSC) + lintian $(DSC) + +$(DSC): $(BUILDDIR) + cd $(BUILDDIR); dpkg-buildpackage -S -us -uc -d + +sbuild: $(DSC) + sbuild $< + +.PHONY: deb dsc +deb: $(OTHER_DEBS) +$(OTHER_DEBS): $(MAIN_DEB) +$(MAIN_DEB): $(BUILDDIR) + cd $(BUILDDIR); dpkg-buildpackage -b -us -uc + lintian $(DEBS) + +distclean: clean +clean: + cargo clean + rm -rf $(PACKAGE)-[0-9]*/ + rm -r *.deb *.dsc $(DEB_SOURCE)*.tar* *.build *.buildinfo *.changes Cargo.lock + +.PHONY: dinstall +dinstall: $(DEBS) + dpkg -i $(DEBS) + diff --git a/src/main.rs b/src/main.rs new file mode 100644 index 0000000..1aefd29 --- /dev/null +++ b/src/main.rs @@ -0,0 +1,311 @@ +extern crate anyhow; +extern crate clap; +extern crate proxmox_backup_qemu; +extern crate proxmox_io; +extern crate proxmox_sys; +extern crate scopeguard; + +use std::env; +use std::ffi::{c_char, CStr, CString}; +use std::ptr; +use std::time::{SystemTime, UNIX_EPOCH}; + +use anyhow::{anyhow, Context, Result}; +use clap::{command, Arg, ArgAction}; +use proxmox_backup_qemu::*; +use proxmox_sys::linux::tty; +use scopeguard::defer; + +mod vma; +use vma::*; + +fn backup_vma_to_pbs( + vma_file_path: String, + pbs_repository: String, + backup_id: String, + pbs_password: String, + keyfile: Option<String>, + key_password: Option<String>, + master_keyfile: Option<String>, + fingerprint: String, + compress: bool, + encrypt: bool, +) -> Result<()> { + println!("VMA input file: {}", vma_file_path); + println!("PBS repository: {}", pbs_repository); + println!("PBS fingerprint: {}", fingerprint); + println!("compress: {}", compress); + println!("encrypt: {}", encrypt); + + let backup_time = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(); + println!("backup time: {}", backup_time); + + let mut pbs_err: *mut c_char = ptr::null_mut(); + + let pbs_repository_cstr = CString::new(pbs_repository).unwrap(); + let backup_id_cstr = CString::new(backup_id).unwrap(); + let pbs_password_cstr = CString::new(pbs_password).unwrap(); + let fingerprint_cstr = CString::new(fingerprint).unwrap(); + let keyfile_cstr = keyfile.map(|v| CString::new(v).unwrap()); + let keyfile_ptr = keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); + let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); + let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); + let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); + let master_keyfile_ptr = master_keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); + + let pbs = proxmox_backup_new_ns( + pbs_repository_cstr.as_ptr(), + ptr::null(), + backup_id_cstr.as_ptr(), + backup_time, + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, + pbs_password_cstr.as_ptr(), + keyfile_ptr, + key_password_ptr, + master_keyfile_ptr, + true, + false, + fingerprint_cstr.as_ptr(), + &mut pbs_err, + ); + + defer! { + proxmox_backup_disconnect(pbs); + } + + if pbs == ptr::null_mut() { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}")); + } + } + + let connect_result = proxmox_backup_connect(pbs, &mut pbs_err); + + if connect_result < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!("proxmox_backup_connect failed: {pbs_err_cstr:?}")); + } + } + + let mut vma_reader = VmaReader::new(&vma_file_path)?; + + // Handle configs + let configs = vma_reader.get_configs(); + for (config_name, config_data) in configs { + println!("CFG: size: {} name: {}", config_data.len(), config_name); + + let config_name_cstr = CString::new(config_name).unwrap(); + + if proxmox_backup_add_config( + pbs, + config_name_cstr.as_ptr(), + config_data.as_ptr(), + config_data.len() as u64, + &mut pbs_err, + ) < 0 + { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_add_config failed: {pbs_err_cstr:?}" + )); + } + } + } + + // Handle block devices + for device_id in 0..255 { + let device_name = match vma_reader.get_device_name(device_id) { + Some(x) => x, + None => { + continue; + } + }; + + let device_size = match vma_reader.get_device_size(device_id) { + Some(x) => x, + None => { + continue; + } + }; + + println!( + "DEV: dev_id={} size: {} devname: {}", + device_id, device_size, device_name + ); + + let device_name_cstr = CString::new(device_name).unwrap(); + let pbs_device_id = proxmox_backup_register_image( + pbs, + device_name_cstr.as_ptr(), + device_size, + false, + &mut pbs_err, + ); + + if pbs_device_id < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_register_image failed: {pbs_err_cstr:?}" + )); + } + } + + let mut image_chunk_buffer = proxmox_io::boxed::zeroed(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE as usize); + let mut bytes_transferred = 0; + + while bytes_transferred < device_size { + let bytes_left = device_size - bytes_transferred; + let chunk_size = bytes_left.min(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE); + println!( + "Uploading dev_id: {} offset: {:#0X} - {:#0X}", + device_id, + bytes_transferred, + bytes_transferred + chunk_size + ); + + let is_zero_chunk = vma_reader + .read_device_contents( + device_id, + &mut image_chunk_buffer[0..chunk_size as usize], + bytes_transferred, + ) + .with_context(|| { + format!( + "read {} bytes at offset {} from disk {} from VMA file", + chunk_size, bytes_transferred, device_id + ) + })?; + + let write_data_result = proxmox_backup_write_data( + pbs, + pbs_device_id as u8, + if is_zero_chunk { + ptr::null() + } else { + image_chunk_buffer.as_ptr() + }, + bytes_transferred, + chunk_size, + &mut pbs_err, + ); + + if write_data_result < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_write_data failed: {pbs_err_cstr:?}" + )); + } + } + + bytes_transferred += chunk_size; + } + + if proxmox_backup_close_image(pbs, pbs_device_id as u8, &mut pbs_err) < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_close_image failed: {pbs_err_cstr:?}" + )); + } + } + } + + if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!("proxmox_backup_finish failed: {pbs_err_cstr:?}")); + } + } + + Ok(()) +} + +fn main() -> Result<()> { + let matches = command!() + .arg( + Arg::new("repository") + .long("repository") + .value_name("auth_id@host:port:datastore") + .help("Repository URL") + .required(true), + ) + .arg( + Arg::new("vmid") + .long("vmid") + .value_name("VMID") + .help("Backup ID") + .required(true), + ) + .arg( + Arg::new("fingerprint") + .long("fingerprint") + .value_name("FINGERPRINT") + .help("Proxmox Backup Server Fingerprint") + .required(true), + ) + .arg( + Arg::new("keyfile") + .long("keyfile") + .value_name("KEYFILE") + .help("Key file"), + ) + .arg( + Arg::new("master_keyfile") + .long("master_keyfile") + .value_name("MASTER_KEYFILE") + .help("Master key file"), + ) + .arg( + Arg::new("compress") + .long("compress") + .short('c') + .help("Compress the Backup") + .action(ArgAction::SetTrue), + ) + .arg( + Arg::new("encrypt") + .long("encrypt") + .short('e') + .help("Encrypt the Backup") + .action(ArgAction::SetTrue), + ) + .arg(Arg::new("vma_file")) + .get_matches(); + + let pbs_repository = matches.get_one::<String>("repository").unwrap().to_string(); + let vmid = matches.get_one::<String>("vmid").unwrap().to_string(); + let fingerprint = matches.get_one::<String>("fingerprint").unwrap().to_string(); + + let keyfile = matches.get_one::<String>("keyfile"); + let master_keyfile = matches.get_one::<String>("master_keyfile"); + let compress = matches.get_flag("compress"); + let encrypt = matches.get_flag("encrypt"); + + let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string(); + + let pbs_password = String::from_utf8(tty::read_password(&"Password: ").unwrap()).unwrap(); + let key_password = match keyfile { + Some(_) => Some(String::from_utf8(tty::read_password(&"Key Password: ").unwrap()).unwrap()), + None => None, + }; + + backup_vma_to_pbs( + vma_file_path, + pbs_repository, + vmid, + pbs_password, + keyfile.cloned(), + key_password, + master_keyfile.cloned(), + fingerprint, + compress, + encrypt, + )?; + + Ok(()) +} diff --git a/src/vma.rs b/src/vma.rs new file mode 100644 index 0000000..e2c3475 --- /dev/null +++ b/src/vma.rs @@ -0,0 +1,340 @@ +extern crate anyhow; +extern crate md5; + +use std::collections::HashMap; +use std::fs::File; +use std::io::{Read, Seek, SeekFrom}; +use std::mem::size_of; +use std::{cmp, str}; + +use anyhow::{anyhow, Result}; +use bincode::Options; +use serde::{Deserialize, Serialize}; +use serde_big_array::BigArray; + +const VMA_BLOCKS_PER_EXTENT: usize = 59; +const VMA_MAX_CONFIGS: usize = 256; +const VMA_MAX_DEVICES: usize = 256; + +#[repr(C)] +#[derive(Serialize, Deserialize)] +struct VmaDeviceInfoHeader { + pub device_name_offset: u32, + reserved: [u8; 4], + pub device_size: u64, + reserved1: [u8; 16], +} + +#[repr(C)] +#[derive(Serialize, Deserialize)] +struct VmaHeader { + pub magic: [u8; 4], + pub version: u32, + pub uuid: [u8; 16], + pub ctime: u64, + pub md5sum: [u8; 16], + pub blob_buffer_offset: u32, + pub blob_buffer_size: u32, + pub header_size: u32, + #[serde(with = "BigArray")] + reserved: [u8; 1984], + #[serde(with = "BigArray")] + pub config_names: [u32; VMA_MAX_CONFIGS], + #[serde(with = "BigArray")] + pub config_data: [u32; VMA_MAX_CONFIGS], + reserved1: [u8; 4], + #[serde(with = "BigArray")] + pub dev_info: [VmaDeviceInfoHeader; VMA_MAX_DEVICES], +} + +#[repr(C)] +#[derive(Serialize, Deserialize)] +struct VmaBlockInfo { + pub mask: u16, + reserved: u8, + pub dev_id: u8, + pub cluster_num: u32, +} + +#[repr(C)] +#[derive(Serialize, Deserialize)] +struct VmaExtentHeader { + pub magic: [u8; 4], + reserved: [u8; 2], + pub block_count: u16, + pub uuid: [u8; 16], + pub md5sum: [u8; 16], + #[serde(with = "BigArray")] + pub blockinfo: [VmaBlockInfo; VMA_BLOCKS_PER_EXTENT], +} + +#[derive(Clone)] +struct VmaBlockIndexEntry { + pub cluster_file_offset: u64, + pub mask: u16, +} + +pub struct VmaReader { + vma_file: File, + vma_header: VmaHeader, + configs: HashMap<String, String>, + block_index: Vec<Vec<VmaBlockIndexEntry>>, + blocks_are_indexed: bool, +} + +impl VmaReader { + pub fn new(vma_file_path: &str) -> Result<Self> { + let mut vma_file = match File::open(vma_file_path) { + Err(why) => return Err(anyhow!("couldn't open {}: {}", vma_file_path, why)), + Ok(file) => file, + }; + + let vma_header = Self::read_header(&mut vma_file).unwrap(); + let configs = Self::read_blob_buffer(&mut vma_file, &vma_header).unwrap(); + let block_index: Vec<Vec<VmaBlockIndexEntry>> = (0..256).map(|_| Vec::new()).collect(); + + let instance = Self { + vma_file, + vma_header, + configs, + block_index, + blocks_are_indexed: false, + }; + + Ok(instance) + } + + fn read_header(vma_file: &mut File) -> Result<VmaHeader> { + let mut buffer = Vec::with_capacity(size_of::<VmaHeader>()); + buffer.resize(size_of::<VmaHeader>(), 0); + vma_file.read_exact(&mut buffer)?; + + let bincode_options = bincode::DefaultOptions::new() + .with_fixint_encoding() + .with_big_endian(); + + let vma_header: VmaHeader = bincode_options.deserialize(&buffer)?; + + if vma_header.magic != [b'V', b'M', b'A', 0] { + return Err(anyhow!("Invalid magic number")); + } + + if vma_header.version != 1 { + return Err(anyhow!("Invalid VMA version {}", vma_header.version)); + } + + buffer.resize(vma_header.header_size as usize, 0); + vma_file.read_exact(&mut buffer[size_of::<VmaHeader>()..])?; + + // Fill the MD5 sum field with zeros to compute the MD5 sum + buffer[32..48].fill(0); + let computed_md5sum: [u8; 16] = md5::compute(&buffer).into(); + + if vma_header.md5sum != computed_md5sum { + return Err(anyhow!("Wrong VMA header checksum")); + } + + return Ok(vma_header); + } + + fn read_string_from_file(vma_file: &mut File, file_offset: u64) -> Result<String> { + let mut size_bytes = [0u8; 2]; + vma_file.seek(SeekFrom::Start(file_offset))?; + vma_file.read_exact(&mut size_bytes)?; + let size = u16::from_le_bytes(size_bytes) as usize; + let mut string_bytes = Vec::with_capacity(size - 1); + string_bytes.resize(size - 1, 0); + vma_file.read_exact(&mut string_bytes)?; + let string = str::from_utf8(&string_bytes)?; + + return Ok(string.to_string()); + } + + fn read_blob_buffer( + vma_file: &mut File, + vma_header: &VmaHeader, + ) -> Result<HashMap<String, String>> { + let mut configs = HashMap::new(); + + for i in 0..VMA_MAX_CONFIGS { + let config_name_offset = vma_header.config_names[i]; + let config_data_offset = vma_header.config_data[i]; + + if config_name_offset == 0 || config_data_offset == 0 { + continue; + } + + let config_name_file_offset = (vma_header.blob_buffer_offset + config_name_offset) as u64; + let config_data_file_offset = (vma_header.blob_buffer_offset + config_data_offset) as u64; + let config_name = Self::read_string_from_file(vma_file, config_name_file_offset)?; + let config_data = Self::read_string_from_file(vma_file, config_data_file_offset)?; + + configs.insert(String::from(config_name), String::from(config_data)); + } + + return Ok(configs); + } + + pub fn get_configs(&self) -> HashMap<String, String> { + return self.configs.clone(); + } + + pub fn get_device_name(&mut self, device_id: usize) -> Option<String> { + if device_id >= VMA_MAX_DEVICES { + return None; + } + + let device_name_offset = self.vma_header.dev_info[device_id].device_name_offset; + + if device_name_offset == 0 { + return None; + } + + let device_name_file_offset = (self.vma_header.blob_buffer_offset + device_name_offset) as u64; + let device_name = Self::read_string_from_file(&mut self.vma_file, device_name_file_offset).unwrap(); + + return Some(device_name.to_string()); + } + + pub fn get_device_size(&self, device_id: usize) -> Option<u64> { + if device_id >= VMA_MAX_DEVICES { + return None; + } + + let dev_info = &self.vma_header.dev_info[device_id]; + + if dev_info.device_name_offset == 0 { + return None; + } + + return Some(dev_info.device_size); + } + + fn read_extent_header(vma_file: &mut File) -> Result<VmaExtentHeader> { + let mut buffer = Vec::with_capacity(size_of::<VmaExtentHeader>()); + buffer.resize(size_of::<VmaExtentHeader>(), 0); + vma_file.read_exact(&mut buffer)?; + + let bincode_options = bincode::DefaultOptions::new() + .with_fixint_encoding() + .with_big_endian(); + + let vma_extent_header: VmaExtentHeader = bincode_options.deserialize(&buffer)?; + + if vma_extent_header.magic != [b'V', b'M', b'A', b'E'] { + return Err(anyhow!("Invalid magic number")); + } + + // Fill the MD5 sum field with zeros to compute the MD5 sum + buffer[24..40].fill(0); + let computed_md5sum: [u8; 16] = md5::compute(&buffer).into(); + + if vma_extent_header.md5sum != computed_md5sum { + return Err(anyhow!("Wrong VMA extent header checksum")); + } + + return Ok(vma_extent_header); + } + + fn index_device_clusters(&mut self) -> Result<()> { + for device_id in 0..255 { + let device_size = match self.get_device_size(device_id) { + Some(x) => x, + None => { + continue; + } + }; + + let device_cluster_count = (device_size + 4096 * 16 - 1) / (4096 * 16); + + let block_index_entry_placeholder = VmaBlockIndexEntry { + cluster_file_offset: 0, + mask: 0, + }; + + self.block_index[device_id].resize(device_cluster_count as usize, block_index_entry_placeholder); + } + + let mut file_offset = self.vma_header.header_size as u64; + let vma_file_size = self.vma_file.metadata()?.len(); + + while file_offset < vma_file_size { + self.vma_file.seek(SeekFrom::Start(file_offset))?; + let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; + file_offset += size_of::<VmaExtentHeader>() as u64; + + for i in 0..VMA_BLOCKS_PER_EXTENT { + let blockinfo = &vma_extent_header.blockinfo[i]; + + if blockinfo.dev_id == 0 { + continue; + } + + let block_index_entry = VmaBlockIndexEntry { + cluster_file_offset: file_offset, + mask: blockinfo.mask, + }; + + self.block_index[blockinfo.dev_id as usize][blockinfo.cluster_num as usize] = block_index_entry; + file_offset += blockinfo.mask.count_ones() as u64 * 4096; + } + } + + self.blocks_are_indexed = true; + + return Ok(()); + } + + pub fn read_device_contents( + &mut self, + device_id: usize, + buffer: &mut [u8], + offset: u64, + ) -> Result<bool> { + if device_id >= VMA_MAX_DEVICES { + return Err(anyhow!("invalid device id {}", device_id)); + } + + if offset % (4096 * 16) != 0 { + return Err(anyhow!("offset is not aligned to 65536")); + } + + // Make sure that the device clusters are already indexed + if !self.blocks_are_indexed { + self.index_device_clusters()?; + } + + let this_device_block_index = &self.block_index[device_id]; + let length = cmp::min( + buffer.len(), + this_device_block_index.len() * 4096 * 16 - offset as usize, + ); + let mut buffer_offset = 0; + let mut buffer_is_zero = true; + + while buffer_offset < length { + let block_index_entry = &this_device_block_index[(offset as usize + buffer_offset) / (4096 * 16)]; + self.vma_file.seek(SeekFrom::Start(block_index_entry.cluster_file_offset))?; + + for i in 0..16 { + if buffer_offset >= length { + break; + } + + let block_buffer_end = buffer_offset + cmp::min(length - buffer_offset, 4096); + let block_mask = ((block_index_entry.mask >> i) & 1) == 1; + + if block_mask { + self.vma_file.read_exact(&mut buffer[buffer_offset..block_buffer_end])?; + buffer_is_zero = false; + } else { + buffer[buffer_offset..block_buffer_end].fill(0); + } + + buffer_offset += 4096; + } + } + + return Ok(buffer_is_zero); + } +} diff --git a/submodules/proxmox-backup-qemu b/submodules/proxmox-backup-qemu new file mode 160000 index 0000000..8af623b --- /dev/null +++ b/submodules/proxmox-backup-qemu @@ -0,0 +1 @@ +Subproject commit 8af623b2100bcda171074addbcb27d828bed2e99 -- 2.39.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit Filip Schauer @ 2024-03-06 15:49 ` Max Carrara 0 siblings, 0 replies; 14+ messages in thread From: Max Carrara @ 2024-03-06 15:49 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Filip Schauer On 3/5/24 14:56, Filip Schauer wrote: > Implement a tool to import VMA files into a Proxmox Backup Server > > Signed-off-by: Filip Schauer <f.schauer@proxmox.com> > --- I know that this was applied already, but please see the comments inline regardless. > .cargo/config | 5 + > .gitmodules | 3 + > Cargo.toml | 19 ++ > Makefile | 70 +++++++ > src/main.rs | 311 ++++++++++++++++++++++++++++++ > src/vma.rs | 340 +++++++++++++++++++++++++++++++++ > submodules/proxmox-backup-qemu | 1 + > 7 files changed, 749 insertions(+) > create mode 100644 .cargo/config > create mode 100644 .gitmodules > create mode 100644 Cargo.toml > create mode 100644 Makefile > create mode 100644 src/main.rs > create mode 100644 src/vma.rs > create mode 160000 submodules/proxmox-backup-qemu > > diff --git a/.cargo/config b/.cargo/config > new file mode 100644 > index 0000000..3b5b6e4 > --- /dev/null > +++ b/.cargo/config > @@ -0,0 +1,5 @@ > +[source] > +[source.debian-packages] > +directory = "/usr/share/cargo/registry" > +[source.crates-io] > +replace-with = "debian-packages" > diff --git a/.gitmodules b/.gitmodules > new file mode 100644 > index 0000000..6d21e06 > --- /dev/null > +++ b/.gitmodules > @@ -0,0 +1,3 @@ > +[submodule "submodules/proxmox-backup-qemu"] > + path = submodules/proxmox-backup-qemu > + url = git://git.proxmox.com/git/proxmox-backup-qemu.git > diff --git a/Cargo.toml b/Cargo.toml > new file mode 100644 > index 0000000..9711690 > --- /dev/null > +++ b/Cargo.toml > @@ -0,0 +1,19 @@ > +[package] > +name = "vma-to-pbs" > +version = "0.0.1" > +authors = ["Filip Schauer <f.schauer@proxmox.com>"] > +edition = "2021" > + > +[dependencies] > +anyhow = "1.0" > +bincode = "1.3" > +clap = { version = "4.0.32", features = ["cargo"] } > +md5 = "0.7.0" > +scopeguard = "1.1.0" > +serde = "1.0" > +serde-big-array = "0.4.1" > + > +proxmox-io = "1.0.1" > +proxmox-sys = "0.5.0" > + > +proxmox-backup-qemu = { path = "submodules/proxmox-backup-qemu" } > diff --git a/Makefile b/Makefile > new file mode 100644 > index 0000000..a0c841d > --- /dev/null > +++ b/Makefile > @@ -0,0 +1,70 @@ > +include /usr/share/dpkg/default.mk > + > +PACKAGE = proxmox-vma-to-pbs > +BUILDDIR = $(PACKAGE)-$(DEB_VERSION_UPSTREAM) > + > +ARCH := $(DEB_BUILD_ARCH) > + > +DSC=$(DEB_SOURCE)_$(DEB_VERSION).dsc > +MAIN_DEB=$(PACKAGE)_$(DEB_VERSION)_$(ARCH).deb > +OTHER_DEBS = \ > + $(PACKAGE)-dev_$(DEB_VERSION)_$(ARCH).deb \ > + $(PACKAGE)-dbgsym_$(DEB_VERSION)_$(ARCH).deb > +DEBS=$(MAIN_DEB) $(OTHER_DEBS) > + > +DESTDIR= > + > +TARGET_DIR := target/debug > + > +ifeq ($(BUILD_MODE), release) > +CARGO_BUILD_ARGS += --release > +TARGETDIR := target/release > +endif > + > +.PHONY: all build > +all: build > + > +build: $(TARGETDIR)/vma-to-pbs > +$(TARGETDIR)/vma-to-pbs: Cargo.toml src/ > + cargo build $(CARGO_BUILD_ARGS) > + > +.PHONY: install > +install: $(TARGETDIR)/vma-to-pbs > + install -D -m 0755 $(TARGETDIR)/vma-to-pbs $(DESTDIR)/usr/bin/vma-to-pbs > + > +$(BUILDDIR): submodule > + rm -rf $@ $@.tmp && mkdir $@.tmp > + cp -a submodules debian Makefile .cargo Cargo.toml build.rs src $@.tmp/ > + mv $@.tmp $@ > + > +submodule: > + [ -e submodules/proxmox-backup-qemu/Cargo.toml ] || [ -e submodules/proxmox/proxmox-sys/Cargo.toml ] || git submodule update --init --recursive > + > +dsc: > + rm -rf $(BUILDDIR) $(DSC) > + $(MAKE) $(DSC) > + lintian $(DSC) > + > +$(DSC): $(BUILDDIR) > + cd $(BUILDDIR); dpkg-buildpackage -S -us -uc -d > + > +sbuild: $(DSC) > + sbuild $< > + > +.PHONY: deb dsc > +deb: $(OTHER_DEBS) > +$(OTHER_DEBS): $(MAIN_DEB) > +$(MAIN_DEB): $(BUILDDIR) > + cd $(BUILDDIR); dpkg-buildpackage -b -us -uc > + lintian $(DEBS) > + > +distclean: clean > +clean: > + cargo clean > + rm -rf $(PACKAGE)-[0-9]*/ > + rm -r *.deb *.dsc $(DEB_SOURCE)*.tar* *.build *.buildinfo *.changes Cargo.lock > + > +.PHONY: dinstall > +dinstall: $(DEBS) > + dpkg -i $(DEBS) > + > diff --git a/src/main.rs b/src/main.rs > new file mode 100644 > index 0000000..1aefd29 > --- /dev/null > +++ b/src/main.rs > @@ -0,0 +1,311 @@ > +extern crate anyhow; > +extern crate clap; > +extern crate proxmox_backup_qemu; > +extern crate proxmox_io; > +extern crate proxmox_sys; > +extern crate scopeguard; `extern crate` declarations are unnecessary, as this repo is on edition 2021 (see above). The `extern crate` declarations became unnecessary with edition 2018. [0] > + > +use std::env; > +use std::ffi::{c_char, CStr, CString}; > +use std::ptr; > +use std::time::{SystemTime, UNIX_EPOCH}; > + > +use anyhow::{anyhow, Context, Result}; > +use clap::{command, Arg, ArgAction}; > +use proxmox_backup_qemu::*; > +use proxmox_sys::linux::tty; > +use scopeguard::defer; > + > +mod vma; > +use vma::*; > + Regarding the function below - see my reply in patch 5. Since it's already been changed since this commit, there's no point in commenting here. > +fn backup_vma_to_pbs( > + vma_file_path: String, > + pbs_repository: String, > + backup_id: String, > + pbs_password: String, > + keyfile: Option<String>, > + key_password: Option<String>, > + master_keyfile: Option<String>, > + fingerprint: String, > + compress: bool, > + encrypt: bool, > +) -> Result<()> { > + println!("VMA input file: {}", vma_file_path); > + println!("PBS repository: {}", pbs_repository); > + println!("PBS fingerprint: {}", fingerprint); > + println!("compress: {}", compress); > + println!("encrypt: {}", encrypt); > + > + let backup_time = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(); > + println!("backup time: {}", backup_time); > + > + let mut pbs_err: *mut c_char = ptr::null_mut(); > + > + let pbs_repository_cstr = CString::new(pbs_repository).unwrap(); > + let backup_id_cstr = CString::new(backup_id).unwrap(); > + let pbs_password_cstr = CString::new(pbs_password).unwrap(); > + let fingerprint_cstr = CString::new(fingerprint).unwrap(); > + let keyfile_cstr = keyfile.map(|v| CString::new(v).unwrap()); > + let keyfile_ptr = keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > + let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); > + let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > + let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); > + let master_keyfile_ptr = master_keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > + > + let pbs = proxmox_backup_new_ns( > + pbs_repository_cstr.as_ptr(), > + ptr::null(), > + backup_id_cstr.as_ptr(), > + backup_time, > + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, > + pbs_password_cstr.as_ptr(), > + keyfile_ptr, > + key_password_ptr, > + master_keyfile_ptr, > + true, > + false, > + fingerprint_cstr.as_ptr(), > + &mut pbs_err, > + ); > + > + defer! { > + proxmox_backup_disconnect(pbs); > + } > + > + if pbs == ptr::null_mut() { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}")); > + } > + } > + > + let connect_result = proxmox_backup_connect(pbs, &mut pbs_err); > + > + if connect_result < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!("proxmox_backup_connect failed: {pbs_err_cstr:?}")); > + } > + } > + > + let mut vma_reader = VmaReader::new(&vma_file_path)?; > + > + // Handle configs > + let configs = vma_reader.get_configs(); > + for (config_name, config_data) in configs { > + println!("CFG: size: {} name: {}", config_data.len(), config_name); > + > + let config_name_cstr = CString::new(config_name).unwrap(); > + > + if proxmox_backup_add_config( > + pbs, > + config_name_cstr.as_ptr(), > + config_data.as_ptr(), > + config_data.len() as u64, > + &mut pbs_err, > + ) < 0 > + { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_add_config failed: {pbs_err_cstr:?}" > + )); > + } > + } > + } > + > + // Handle block devices > + for device_id in 0..255 { > + let device_name = match vma_reader.get_device_name(device_id) { > + Some(x) => x, > + None => { > + continue; > + } > + }; > + > + let device_size = match vma_reader.get_device_size(device_id) { > + Some(x) => x, > + None => { > + continue; > + } > + }; > + > + println!( > + "DEV: dev_id={} size: {} devname: {}", > + device_id, device_size, device_name > + ); > + > + let device_name_cstr = CString::new(device_name).unwrap(); > + let pbs_device_id = proxmox_backup_register_image( > + pbs, > + device_name_cstr.as_ptr(), > + device_size, > + false, > + &mut pbs_err, > + ); > + > + if pbs_device_id < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_register_image failed: {pbs_err_cstr:?}" > + )); > + } > + } > + > + let mut image_chunk_buffer = proxmox_io::boxed::zeroed(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE as usize); > + let mut bytes_transferred = 0; > + > + while bytes_transferred < device_size { > + let bytes_left = device_size - bytes_transferred; > + let chunk_size = bytes_left.min(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE); > + println!( > + "Uploading dev_id: {} offset: {:#0X} - {:#0X}", > + device_id, > + bytes_transferred, > + bytes_transferred + chunk_size > + ); > + > + let is_zero_chunk = vma_reader > + .read_device_contents( > + device_id, > + &mut image_chunk_buffer[0..chunk_size as usize], > + bytes_transferred, > + ) > + .with_context(|| { > + format!( > + "read {} bytes at offset {} from disk {} from VMA file", > + chunk_size, bytes_transferred, device_id > + ) > + })?; > + > + let write_data_result = proxmox_backup_write_data( > + pbs, > + pbs_device_id as u8, > + if is_zero_chunk { > + ptr::null() > + } else { > + image_chunk_buffer.as_ptr() > + }, > + bytes_transferred, > + chunk_size, > + &mut pbs_err, > + ); > + > + if write_data_result < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_write_data failed: {pbs_err_cstr:?}" > + )); > + } > + } > + > + bytes_transferred += chunk_size; > + } > + > + if proxmox_backup_close_image(pbs, pbs_device_id as u8, &mut pbs_err) < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_close_image failed: {pbs_err_cstr:?}" > + )); > + } > + } > + } > + > + if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!("proxmox_backup_finish failed: {pbs_err_cstr:?}")); > + } > + } > + > + Ok(()) > +} > + > +fn main() -> Result<()> { > + let matches = command!() > + .arg( > + Arg::new("repository") > + .long("repository") > + .value_name("auth_id@host:port:datastore") > + .help("Repository URL") > + .required(true), > + ) > + .arg( > + Arg::new("vmid") > + .long("vmid") > + .value_name("VMID") > + .help("Backup ID") > + .required(true), > + ) > + .arg( > + Arg::new("fingerprint") > + .long("fingerprint") > + .value_name("FINGERPRINT") > + .help("Proxmox Backup Server Fingerprint") > + .required(true), > + ) > + .arg( > + Arg::new("keyfile") > + .long("keyfile") > + .value_name("KEYFILE") > + .help("Key file"), > + ) > + .arg( > + Arg::new("master_keyfile") > + .long("master_keyfile") > + .value_name("MASTER_KEYFILE") > + .help("Master key file"), > + ) > + .arg( > + Arg::new("compress") > + .long("compress") > + .short('c') > + .help("Compress the Backup") > + .action(ArgAction::SetTrue), > + ) > + .arg( > + Arg::new("encrypt") > + .long("encrypt") > + .short('e') > + .help("Encrypt the Backup") > + .action(ArgAction::SetTrue), > + ) > + .arg(Arg::new("vma_file")) > + .get_matches(); > + > + let pbs_repository = matches.get_one::<String>("repository").unwrap().to_string(); > + let vmid = matches.get_one::<String>("vmid").unwrap().to_string(); > + let fingerprint = matches.get_one::<String>("fingerprint").unwrap().to_string(); > + > + let keyfile = matches.get_one::<String>("keyfile"); > + let master_keyfile = matches.get_one::<String>("master_keyfile"); > + let compress = matches.get_flag("compress"); > + let encrypt = matches.get_flag("encrypt"); > + > + let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string(); > + > + let pbs_password = String::from_utf8(tty::read_password(&"Password: ").unwrap()).unwrap(); > + let key_password = match keyfile { > + Some(_) => Some(String::from_utf8(tty::read_password(&"Key Password: ").unwrap()).unwrap()), > + None => None, > + }; These `unwrap()`s shouldn't have been applied in the first place IMHO, but I'm glad you got rid of most of them in subsequent commits. Overall, all these parsed arguments should be represented as a single struct in order to allow it to be passed around easier and make the code vastly more readable - something like `args.vmid` signals clearly that `vmid` is something that came from the arguments the user supplied. It's much more expressive than freestanding arguments / parameters floating around. > + > + backup_vma_to_pbs( > + vma_file_path, > + pbs_repository, > + vmid, > + pbs_password, > + keyfile.cloned(), > + key_password, > + master_keyfile.cloned(), > + fingerprint, > + compress, > + encrypt, > + )?> + > + Ok(()) > +} > diff --git a/src/vma.rs b/src/vma.rs > new file mode 100644 > index 0000000..e2c3475 > --- /dev/null > +++ b/src/vma.rs > @@ -0,0 +1,340 @@ > +extern crate anyhow; > +extern crate md5; Unnecessary `extern crate`s here again. [0] > + > +use std::collections::HashMap; > +use std::fs::File; > +use std::io::{Read, Seek, SeekFrom}; > +use std::mem::size_of; > +use std::{cmp, str}; > + > +use anyhow::{anyhow, Result}; > +use bincode::Options; > +use serde::{Deserialize, Serialize}; > +use serde_big_array::BigArray; > + > +const VMA_BLOCKS_PER_EXTENT: usize = 59; > +const VMA_MAX_CONFIGS: usize = 256; > +const VMA_MAX_DEVICES: usize = 256; Good use of constants here! The definitions of the structs below also look pretty solid, though I would prefer if there were docstrings associated with them. Those docstrings should also then refer to the original C-structs where you got all the original field definitions from. > + > +#[repr(C)] > +#[derive(Serialize, Deserialize)] > +struct VmaDeviceInfoHeader { > + pub device_name_offset: u32, > + reserved: [u8; 4], > + pub device_size: u64, > + reserved1: [u8; 16], > +} > + > +#[repr(C)] > +#[derive(Serialize, Deserialize)] > +struct VmaHeader { > + pub magic: [u8; 4], > + pub version: u32, > + pub uuid: [u8; 16], > + pub ctime: u64, > + pub md5sum: [u8; 16], > + pub blob_buffer_offset: u32, > + pub blob_buffer_size: u32, > + pub header_size: u32, > + #[serde(with = "BigArray")] > + reserved: [u8; 1984], > + #[serde(with = "BigArray")] > + pub config_names: [u32; VMA_MAX_CONFIGS], > + #[serde(with = "BigArray")] > + pub config_data: [u32; VMA_MAX_CONFIGS], > + reserved1: [u8; 4], > + #[serde(with = "BigArray")] > + pub dev_info: [VmaDeviceInfoHeader; VMA_MAX_DEVICES], > +} > + > +#[repr(C)] > +#[derive(Serialize, Deserialize)] > +struct VmaBlockInfo { > + pub mask: u16, > + reserved: u8, > + pub dev_id: u8, > + pub cluster_num: u32, > +} > + > +#[repr(C)] > +#[derive(Serialize, Deserialize)] > +struct VmaExtentHeader { > + pub magic: [u8; 4], > + reserved: [u8; 2], > + pub block_count: u16, > + pub uuid: [u8; 16], > + pub md5sum: [u8; 16], > + #[serde(with = "BigArray")] > + pub blockinfo: [VmaBlockInfo; VMA_BLOCKS_PER_EXTENT], > +} > + > +#[derive(Clone)] > +struct VmaBlockIndexEntry { > + pub cluster_file_offset: u64, > + pub mask: u16, > +} > + > +pub struct VmaReader { > + vma_file: File, > + vma_header: VmaHeader, > + configs: HashMap<String, String>, > + block_index: Vec<Vec<VmaBlockIndexEntry>>, > + blocks_are_indexed: bool, > +} > + > +impl VmaReader { > + pub fn new(vma_file_path: &str) -> Result<Self> { > + let mut vma_file = match File::open(vma_file_path) { > + Err(why) => return Err(anyhow!("couldn't open {}: {}", vma_file_path, why)), Should use `anyhow::bail` [1] here and also inline the args in the format string: bail!("couldn't open {vma_file_path}: {why}") > + Ok(file) => file, > + }; > + > + let vma_header = Self::read_header(&mut vma_file).unwrap(); > + let configs = Self::read_blob_buffer(&mut vma_file, &vma_header).unwrap(); Glad you got rid of the `unwrap()`s above in later commits. > + let block_index: Vec<Vec<VmaBlockIndexEntry>> = (0..256).map(|_| Vec::new()).collect(); > + > + let instance = Self { > + vma_file, > + vma_header, > + configs, > + block_index, > + blocks_are_indexed: false, > + }; > + > + Ok(instance) > + } > + > + fn read_header(vma_file: &mut File) -> Result<VmaHeader> { > + let mut buffer = Vec::with_capacity(size_of::<VmaHeader>()); > + buffer.resize(size_of::<VmaHeader>(), 0); > + vma_file.read_exact(&mut buffer)?; > + > + let bincode_options = bincode::DefaultOptions::new() > + .with_fixint_encoding() > + .with_big_endian(); > + > + let vma_header: VmaHeader = bincode_options.deserialize(&buffer)?; > + > + if vma_header.magic != [b'V', b'M', b'A', 0] { These magic numbers should be a constant as well, e.g.: const MAGIC_NUMBERS: &[&[u8; 1]; 4] = &[b'V', b'M', b'A', 0]; Plus a docstring would be nice. What are they for? Also, if the original data type differs (e.g. it's a u32) it's probably more straighforward to just use that instead. Functions like `from_le_bytes` are const, so they can be used when declaring constants like the one above. > + return Err(anyhow!("Invalid magic number")); Should use `anyhow::bail` [1] instead. > + } > + > + if vma_header.version != 1 { > + return Err(anyhow!("Invalid VMA version {}", vma_header.version)); Should use `anyhow::bail` [1] instead. > + } > + > + buffer.resize(vma_header.header_size as usize, 0); > + vma_file.read_exact(&mut buffer[size_of::<VmaHeader>()..])?; > + > + // Fill the MD5 sum field with zeros to compute the MD5 sum > + buffer[32..48].fill(0); There's no real way to get (C-)struct offsets without using some `unsafe`, so this here is fine. > + let computed_md5sum: [u8; 16] = md5::compute(&buffer).into(); > + > + if vma_header.md5sum != computed_md5sum { > + return Err(anyhow!("Wrong VMA header checksum")); Should use `anyhow::bail` [1] instead. > + } > + > + return Ok(vma_header); > + } > + > + fn read_string_from_file(vma_file: &mut File, file_offset: u64) -> Result<String> { > + let mut size_bytes = [0u8; 2]; > + vma_file.seek(SeekFrom::Start(file_offset))?; > + vma_file.read_exact(&mut size_bytes)?; > + let size = u16::from_le_bytes(size_bytes) as usize; > + let mut string_bytes = Vec::with_capacity(size - 1); > + string_bytes.resize(size - 1, 0); > + vma_file.read_exact(&mut string_bytes)?; > + let string = str::from_utf8(&string_bytes)?; > + > + return Ok(string.to_string()); > + } You changed the function above significantly in a later patch, so won't comment here. > + > + fn read_blob_buffer( > + vma_file: &mut File, > + vma_header: &VmaHeader, > + ) -> Result<HashMap<String, String>> { > + let mut configs = HashMap::new(); > + > + for i in 0..VMA_MAX_CONFIGS { > + let config_name_offset = vma_header.config_names[i]; > + let config_data_offset = vma_header.config_data[i]; > + > + if config_name_offset == 0 || config_data_offset == 0 { > + continue; > + } > + > + let config_name_file_offset = (vma_header.blob_buffer_offset + config_name_offset) as u64; > + let config_data_file_offset = (vma_header.blob_buffer_offset + config_data_offset) as u64; > + let config_name = Self::read_string_from_file(vma_file, config_name_file_offset)?; > + let config_data = Self::read_string_from_file(vma_file, config_data_file_offset)?; > + > + configs.insert(String::from(config_name), String::from(config_data)); > + } > + > + return Ok(configs); > + } Same here. > + > + pub fn get_configs(&self) -> HashMap<String, String> { > + return self.configs.clone(); > + } > + > + pub fn get_device_name(&mut self, device_id: usize) -> Option<String> { > + if device_id >= VMA_MAX_DEVICES { > + return None; > + } > + > + let device_name_offset = self.vma_header.dev_info[device_id].device_name_offset; > + > + if device_name_offset == 0 { > + return None; > + } > + > + let device_name_file_offset = (self.vma_header.blob_buffer_offset + device_name_offset) as u64; > + let device_name = Self::read_string_from_file(&mut self.vma_file, device_name_file_offset).unwrap(); Should return a Result above if the function can fail in more than one way instead of using `unwrap()` - or are you certain that this won't ever fail? > + > + return Some(device_name.to_string()); > + } > + > + pub fn get_device_size(&self, device_id: usize) -> Option<u64> { > + if device_id >= VMA_MAX_DEVICES { > + return None; > + } > + > + let dev_info = &self.vma_header.dev_info[device_id]; > + > + if dev_info.device_name_offset == 0 { > + return None; > + } > + > + return Some(dev_info.device_size); > + } > + > + fn read_extent_header(vma_file: &mut File) -> Result<VmaExtentHeader> { > + let mut buffer = Vec::with_capacity(size_of::<VmaExtentHeader>()); > + buffer.resize(size_of::<VmaExtentHeader>(), 0); > + vma_file.read_exact(&mut buffer)?; > + > + let bincode_options = bincode::DefaultOptions::new() > + .with_fixint_encoding() > + .with_big_endian(); > + > + let vma_extent_header: VmaExtentHeader = bincode_options.deserialize(&buffer)?; > + > + if vma_extent_header.magic != [b'V', b'M', b'A', b'E'] { See above - should be a constant. > + return Err(anyhow!("Invalid magic number")); Should use `anyhow::bail` [1] instead. > + } > + > + // Fill the MD5 sum field with zeros to compute the MD5 sum > + buffer[24..40].fill(0); > + let computed_md5sum: [u8; 16] = md5::compute(&buffer).into(); > + > + if vma_extent_header.md5sum != computed_md5sum { > + return Err(anyhow!("Wrong VMA extent header checksum")); Should use `anyhow::bail` [1] instead. > + } > + > + return Ok(vma_extent_header); > + } > + > + fn index_device_clusters(&mut self) -> Result<()> { > + for device_id in 0..255 { 255 should be a constant - is it among the ones you declared above? > + let device_size = match self.get_device_size(device_id) { > + Some(x) => x, > + None => { > + continue; > + } > + }; > + > + let device_cluster_count = (device_size + 4096 * 16 - 1) / (4096 * 16); I know you changed this later, but (parts of) things like these should *always* be declared as constants, preferably with an explanation / description of that constant as well, if it's not immediately clear (to others!) what the constant stands for. > + > + let block_index_entry_placeholder = VmaBlockIndexEntry { > + cluster_file_offset: 0, > + mask: 0, > + }; > + > + self.block_index[device_id].resize(device_cluster_count as usize, block_index_entry_placeholder); > + } > + > + let mut file_offset = self.vma_header.header_size as u64; > + let vma_file_size = self.vma_file.metadata()?.len(); > + > + while file_offset < vma_file_size { > + self.vma_file.seek(SeekFrom::Start(file_offset))?; > + let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; > + file_offset += size_of::<VmaExtentHeader>() as u64; > + > + for i in 0..VMA_BLOCKS_PER_EXTENT { > + let blockinfo = &vma_extent_header.blockinfo[i]; > + > + if blockinfo.dev_id == 0 { > + continue; > + } > + > + let block_index_entry = VmaBlockIndexEntry { > + cluster_file_offset: file_offset, > + mask: blockinfo.mask, > + }; > + > + self.block_index[blockinfo.dev_id as usize][blockinfo.cluster_num as usize] = block_index_entry; > + file_offset += blockinfo.mask.count_ones() as u64 * 4096; Even things like file block size should be constants, IMO. > + } > + } > + > + self.blocks_are_indexed = true; > + > + return Ok(()); > + } > + > + pub fn read_device_contents( > + &mut self, > + device_id: usize, > + buffer: &mut [u8], > + offset: u64, > + ) -> Result<bool> { > + if device_id >= VMA_MAX_DEVICES { > + return Err(anyhow!("invalid device id {}", device_id)); > + } > + > + if offset % (4096 * 16) != 0 { > + return Err(anyhow!("offset is not aligned to 65536")); > + } Constants would be really useful here. Should also use `anyhow::bail` [1] instead. > + > + // Make sure that the device clusters are already indexed > + if !self.blocks_are_indexed { > + self.index_device_clusters()?; > + } > + > + let this_device_block_index = &self.block_index[device_id]; > + let length = cmp::min( > + buffer.len(), > + this_device_block_index.len() * 4096 * 16 - offset as usize, Again, use case for a constant. > + ); > + let mut buffer_offset = 0; > + let mut buffer_is_zero = true; > + > + while buffer_offset < length { > + let block_index_entry = &this_device_block_index[(offset as usize + buffer_offset) / (4096 * 16)]; > + self.vma_file.seek(SeekFrom::Start(block_index_entry.cluster_file_offset))?; > + > + for i in 0..16 { > + if buffer_offset >= length { > + break; > + } > + > + let block_buffer_end = buffer_offset + cmp::min(length - buffer_offset, 4096); > + let block_mask = ((block_index_entry.mask >> i) & 1) == 1; > + > + if block_mask { > + self.vma_file.read_exact(&mut buffer[buffer_offset..block_buffer_end])?; > + buffer_is_zero = false; > + } else { > + buffer[buffer_offset..block_buffer_end].fill(0); > + } > + > + buffer_offset += 4096; > + } The body of that while-loop could maybe be put into a separate function (maybe even just a nested function since it's not used anywhere else) that describes exactly what's being done here. Also: constants ;) > + } > + > + return Ok(buffer_is_zero); > + } > +} > diff --git a/submodules/proxmox-backup-qemu b/submodules/proxmox-backup-qemu > new file mode 160000 > index 0000000..8af623b > --- /dev/null > +++ b/submodules/proxmox-backup-qemu > @@ -0,0 +1 @@ > +Subproject commit 8af623b2100bcda171074addbcb27d828bed2e99 [0]: https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html [1]: https://docs.rs/anyhow/1.0.80/anyhow/macro.bail.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH v4 vma-to-pbs 2/6] cargo fmt 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit Filip Schauer @ 2024-03-05 13:56 ` Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 3/6] bump proxmox-backup-qemu Filip Schauer ` (5 subsequent siblings) 7 siblings, 0 replies; 14+ messages in thread From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw) To: pbs-devel; +Cc: Wolfgang Bumiller From: Wolfgang Bumiller <w.bumiller@proxmox.com> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- src/main.rs | 17 +++++++++++++---- src/vma.rs | 27 ++++++++++++++++++--------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1aefd29..8d95b11 100644 --- a/src/main.rs +++ b/src/main.rs @@ -37,7 +37,10 @@ fn backup_vma_to_pbs( println!("compress: {}", compress); println!("encrypt: {}", encrypt); - let backup_time = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(); + let backup_time = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs(); println!("backup time: {}", backup_time); let mut pbs_err: *mut c_char = ptr::null_mut(); @@ -51,7 +54,9 @@ fn backup_vma_to_pbs( let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); - let master_keyfile_ptr = master_keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); + let master_keyfile_ptr = master_keyfile_cstr + .map(|v| v.as_ptr()) + .unwrap_or(ptr::null()); let pbs = proxmox_backup_new_ns( pbs_repository_cstr.as_ptr(), @@ -154,7 +159,8 @@ fn backup_vma_to_pbs( } } - let mut image_chunk_buffer = proxmox_io::boxed::zeroed(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE as usize); + let mut image_chunk_buffer = + proxmox_io::boxed::zeroed(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE as usize); let mut bytes_transferred = 0; while bytes_transferred < device_size { @@ -279,7 +285,10 @@ fn main() -> Result<()> { let pbs_repository = matches.get_one::<String>("repository").unwrap().to_string(); let vmid = matches.get_one::<String>("vmid").unwrap().to_string(); - let fingerprint = matches.get_one::<String>("fingerprint").unwrap().to_string(); + let fingerprint = matches + .get_one::<String>("fingerprint") + .unwrap() + .to_string(); let keyfile = matches.get_one::<String>("keyfile"); let master_keyfile = matches.get_one::<String>("master_keyfile"); diff --git a/src/vma.rs b/src/vma.rs index e2c3475..5ec3822 100644 --- a/src/vma.rs +++ b/src/vma.rs @@ -164,8 +164,10 @@ impl VmaReader { continue; } - let config_name_file_offset = (vma_header.blob_buffer_offset + config_name_offset) as u64; - let config_data_file_offset = (vma_header.blob_buffer_offset + config_data_offset) as u64; + let config_name_file_offset = + (vma_header.blob_buffer_offset + config_name_offset) as u64; + let config_data_file_offset = + (vma_header.blob_buffer_offset + config_data_offset) as u64; let config_name = Self::read_string_from_file(vma_file, config_name_file_offset)?; let config_data = Self::read_string_from_file(vma_file, config_data_file_offset)?; @@ -190,8 +192,10 @@ impl VmaReader { return None; } - let device_name_file_offset = (self.vma_header.blob_buffer_offset + device_name_offset) as u64; - let device_name = Self::read_string_from_file(&mut self.vma_file, device_name_file_offset).unwrap(); + let device_name_file_offset = + (self.vma_header.blob_buffer_offset + device_name_offset) as u64; + let device_name = + Self::read_string_from_file(&mut self.vma_file, device_name_file_offset).unwrap(); return Some(device_name.to_string()); } @@ -252,7 +256,8 @@ impl VmaReader { mask: 0, }; - self.block_index[device_id].resize(device_cluster_count as usize, block_index_entry_placeholder); + self.block_index[device_id] + .resize(device_cluster_count as usize, block_index_entry_placeholder); } let mut file_offset = self.vma_header.header_size as u64; @@ -275,7 +280,8 @@ impl VmaReader { mask: blockinfo.mask, }; - self.block_index[blockinfo.dev_id as usize][blockinfo.cluster_num as usize] = block_index_entry; + self.block_index[blockinfo.dev_id as usize][blockinfo.cluster_num as usize] = + block_index_entry; file_offset += blockinfo.mask.count_ones() as u64 * 4096; } } @@ -313,8 +319,10 @@ impl VmaReader { let mut buffer_is_zero = true; while buffer_offset < length { - let block_index_entry = &this_device_block_index[(offset as usize + buffer_offset) / (4096 * 16)]; - self.vma_file.seek(SeekFrom::Start(block_index_entry.cluster_file_offset))?; + let block_index_entry = + &this_device_block_index[(offset as usize + buffer_offset) / (4096 * 16)]; + self.vma_file + .seek(SeekFrom::Start(block_index_entry.cluster_file_offset))?; for i in 0..16 { if buffer_offset >= length { @@ -325,7 +333,8 @@ impl VmaReader { let block_mask = ((block_index_entry.mask >> i) & 1) == 1; if block_mask { - self.vma_file.read_exact(&mut buffer[buffer_offset..block_buffer_end])?; + self.vma_file + .read_exact(&mut buffer[buffer_offset..block_buffer_end])?; buffer_is_zero = false; } else { buffer[buffer_offset..block_buffer_end].fill(0); -- 2.39.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH v4 vma-to-pbs 3/6] bump proxmox-backup-qemu 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 2/6] cargo fmt Filip Schauer @ 2024-03-05 13:56 ` Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 4/6] Add the ability to provide credentials via files Filip Schauer ` (4 subsequent siblings) 7 siblings, 0 replies; 14+ messages in thread From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw) To: pbs-devel; +Cc: Wolfgang Bumiller From: Wolfgang Bumiller <w.bumiller@proxmox.com> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- submodules/proxmox-backup-qemu | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submodules/proxmox-backup-qemu b/submodules/proxmox-backup-qemu index 8af623b..3adc6a1 160000 --- a/submodules/proxmox-backup-qemu +++ b/submodules/proxmox-backup-qemu @@ -1 +1 @@ -Subproject commit 8af623b2100bcda171074addbcb27d828bed2e99 +Subproject commit 3adc6a17877a6bf19a2d04d5fe1dfe6eb62e1eb7 -- 2.39.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH v4 vma-to-pbs 4/6] Add the ability to provide credentials via files 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer ` (2 preceding siblings ...) 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 3/6] bump proxmox-backup-qemu Filip Schauer @ 2024-03-05 13:56 ` Filip Schauer 2024-03-06 15:49 ` Max Carrara 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin Filip Schauer ` (3 subsequent siblings) 7 siblings, 1 reply; 14+ messages in thread From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw) To: pbs-devel Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- src/main.rs | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 8d95b11..b58387b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -280,6 +280,18 @@ fn main() -> Result<()> { .help("Encrypt the Backup") .action(ArgAction::SetTrue), ) + .arg( + Arg::new("password_file") + .long("password_file") + .value_name("PASSWORD_FILE") + .help("Password file"), + ) + .arg( + Arg::new("key_password_file") + .long("key_password_file") + .value_name("KEY_PASSWORD_FILE") + .help("Key password file"), + ) .arg(Arg::new("vma_file")) .get_matches(); @@ -296,10 +308,25 @@ fn main() -> Result<()> { let encrypt = matches.get_flag("encrypt"); let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string(); + let password_file = matches.get_one::<String>("password_file"); + + let pbs_password = match password_file { + Some(password_file) => { + std::fs::read_to_string(password_file).expect("Could not read password file") + } + None => String::from_utf8(tty::read_password("Password: ")?)?, + }; - let pbs_password = String::from_utf8(tty::read_password(&"Password: ").unwrap()).unwrap(); let key_password = match keyfile { - Some(_) => Some(String::from_utf8(tty::read_password(&"Key Password: ").unwrap()).unwrap()), + Some(_) => { + let key_password_file = matches.get_one::<String>("key_password_file"); + + Some(match key_password_file { + Some(key_password_file) => std::fs::read_to_string(key_password_file) + .expect("Could not read key password file"), + None => String::from_utf8(tty::read_password("Key Password: ")?)?, + }) + } None => None, }; -- 2.39.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 4/6] Add the ability to provide credentials via files 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 4/6] Add the ability to provide credentials via files Filip Schauer @ 2024-03-06 15:49 ` Max Carrara 0 siblings, 0 replies; 14+ messages in thread From: Max Carrara @ 2024-03-06 15:49 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Filip Schauer On 3/5/24 14:56, Filip Schauer wrote: > Signed-off-by: Filip Schauer <f.schauer@proxmox.com> > --- > src/main.rs | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/src/main.rs b/src/main.rs > index 8d95b11..b58387b 100644 > --- a/src/main.rs > +++ b/src/main.rs > @@ -280,6 +280,18 @@ fn main() -> Result<()> { > .help("Encrypt the Backup") > .action(ArgAction::SetTrue), > ) > + .arg( > + Arg::new("password_file") > + .long("password_file") > + .value_name("PASSWORD_FILE") > + .help("Password file"), > + ) > + .arg( > + Arg::new("key_password_file") > + .long("key_password_file") > + .value_name("KEY_PASSWORD_FILE") > + .help("Key password file"), > + ) > .arg(Arg::new("vma_file")) > .get_matches(); > > @@ -296,10 +308,25 @@ fn main() -> Result<()> { > let encrypt = matches.get_flag("encrypt"); > > let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string(); > + let password_file = matches.get_one::<String>("password_file"); > + > + let pbs_password = match password_file { > + Some(password_file) => { > + std::fs::read_to_string(password_file).expect("Could not read password file") This here would be better handled by `anyhow::Context` [0]. > + } > + None => String::from_utf8(tty::read_password("Password: ")?)?, > + }; The entire block could *maybe* be something like: let pbs_password = if let Some(filename) = password_file { // ... } else { // ... }; .. as you're only really matching for one variant. > > - let pbs_password = String::from_utf8(tty::read_password(&"Password: ").unwrap()).unwrap(); The `unwrap()`s here should be replaced with a descriptive `anyhow::Context` [0] again, especially since it's UI code. > let key_password = match keyfile { > - Some(_) => Some(String::from_utf8(tty::read_password(&"Key Password: ").unwrap()).unwrap()), Here as well. > + Some(_) => { > + let key_password_file = matches.get_one::<String>("key_password_file"); > + > + Some(match key_password_file { > + Some(key_password_file) => std::fs::read_to_string(key_password_file) > + .expect("Could not read key password file"), The `expect()` here too. > + None => String::from_utf8(tty::read_password("Key Password: ")?)?, > + }) > + } > None => None, > }; There probably isn't any prettier way to do this, so this whole nested match is fine IMO. > [0]: https://docs.rs/anyhow/latest/anyhow/trait.Context.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer ` (3 preceding siblings ...) 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 4/6] Add the ability to provide credentials via files Filip Schauer @ 2024-03-05 13:56 ` Filip Schauer 2024-03-06 15:49 ` Max Carrara 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 6/6] Add a fallback for the --fingerprint argument Filip Schauer ` (2 subsequent siblings) 7 siblings, 1 reply; 14+ messages in thread From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw) To: pbs-devel This allows the user to stream a compressed VMA file directly to a PBS without the need to extract it to an intermediate .vma file. Example usage: zstd -d --stdout vzdump.vma.zstd | vma-to-pbs \ --repository <auth_id@host:port:datastore> \ --vmid 123 \ --password_file pbs_password Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- src/main.rs | 232 +-------------------------------- src/vma.rs | 270 ++++++++++++++------------------------ src/vma2pbs.rs | 348 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 455 insertions(+), 395 deletions(-) create mode 100644 src/vma2pbs.rs diff --git a/src/main.rs b/src/main.rs index b58387b..dc8171e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,235 +1,14 @@ extern crate anyhow; extern crate clap; -extern crate proxmox_backup_qemu; -extern crate proxmox_io; extern crate proxmox_sys; -extern crate scopeguard; -use std::env; -use std::ffi::{c_char, CStr, CString}; -use std::ptr; -use std::time::{SystemTime, UNIX_EPOCH}; - -use anyhow::{anyhow, Context, Result}; +use anyhow::Result; use clap::{command, Arg, ArgAction}; -use proxmox_backup_qemu::*; use proxmox_sys::linux::tty; -use scopeguard::defer; mod vma; -use vma::*; - -fn backup_vma_to_pbs( - vma_file_path: String, - pbs_repository: String, - backup_id: String, - pbs_password: String, - keyfile: Option<String>, - key_password: Option<String>, - master_keyfile: Option<String>, - fingerprint: String, - compress: bool, - encrypt: bool, -) -> Result<()> { - println!("VMA input file: {}", vma_file_path); - println!("PBS repository: {}", pbs_repository); - println!("PBS fingerprint: {}", fingerprint); - println!("compress: {}", compress); - println!("encrypt: {}", encrypt); - - let backup_time = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_secs(); - println!("backup time: {}", backup_time); - - let mut pbs_err: *mut c_char = ptr::null_mut(); - - let pbs_repository_cstr = CString::new(pbs_repository).unwrap(); - let backup_id_cstr = CString::new(backup_id).unwrap(); - let pbs_password_cstr = CString::new(pbs_password).unwrap(); - let fingerprint_cstr = CString::new(fingerprint).unwrap(); - let keyfile_cstr = keyfile.map(|v| CString::new(v).unwrap()); - let keyfile_ptr = keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); - let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); - let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); - let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); - let master_keyfile_ptr = master_keyfile_cstr - .map(|v| v.as_ptr()) - .unwrap_or(ptr::null()); - - let pbs = proxmox_backup_new_ns( - pbs_repository_cstr.as_ptr(), - ptr::null(), - backup_id_cstr.as_ptr(), - backup_time, - PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, - pbs_password_cstr.as_ptr(), - keyfile_ptr, - key_password_ptr, - master_keyfile_ptr, - true, - false, - fingerprint_cstr.as_ptr(), - &mut pbs_err, - ); - - defer! { - proxmox_backup_disconnect(pbs); - } - - if pbs == ptr::null_mut() { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}")); - } - } - - let connect_result = proxmox_backup_connect(pbs, &mut pbs_err); - - if connect_result < 0 { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!("proxmox_backup_connect failed: {pbs_err_cstr:?}")); - } - } - - let mut vma_reader = VmaReader::new(&vma_file_path)?; - - // Handle configs - let configs = vma_reader.get_configs(); - for (config_name, config_data) in configs { - println!("CFG: size: {} name: {}", config_data.len(), config_name); - - let config_name_cstr = CString::new(config_name).unwrap(); - - if proxmox_backup_add_config( - pbs, - config_name_cstr.as_ptr(), - config_data.as_ptr(), - config_data.len() as u64, - &mut pbs_err, - ) < 0 - { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!( - "proxmox_backup_add_config failed: {pbs_err_cstr:?}" - )); - } - } - } - - // Handle block devices - for device_id in 0..255 { - let device_name = match vma_reader.get_device_name(device_id) { - Some(x) => x, - None => { - continue; - } - }; - - let device_size = match vma_reader.get_device_size(device_id) { - Some(x) => x, - None => { - continue; - } - }; - - println!( - "DEV: dev_id={} size: {} devname: {}", - device_id, device_size, device_name - ); - - let device_name_cstr = CString::new(device_name).unwrap(); - let pbs_device_id = proxmox_backup_register_image( - pbs, - device_name_cstr.as_ptr(), - device_size, - false, - &mut pbs_err, - ); - - if pbs_device_id < 0 { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!( - "proxmox_backup_register_image failed: {pbs_err_cstr:?}" - )); - } - } - - let mut image_chunk_buffer = - proxmox_io::boxed::zeroed(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE as usize); - let mut bytes_transferred = 0; - - while bytes_transferred < device_size { - let bytes_left = device_size - bytes_transferred; - let chunk_size = bytes_left.min(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE); - println!( - "Uploading dev_id: {} offset: {:#0X} - {:#0X}", - device_id, - bytes_transferred, - bytes_transferred + chunk_size - ); - - let is_zero_chunk = vma_reader - .read_device_contents( - device_id, - &mut image_chunk_buffer[0..chunk_size as usize], - bytes_transferred, - ) - .with_context(|| { - format!( - "read {} bytes at offset {} from disk {} from VMA file", - chunk_size, bytes_transferred, device_id - ) - })?; - - let write_data_result = proxmox_backup_write_data( - pbs, - pbs_device_id as u8, - if is_zero_chunk { - ptr::null() - } else { - image_chunk_buffer.as_ptr() - }, - bytes_transferred, - chunk_size, - &mut pbs_err, - ); - - if write_data_result < 0 { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!( - "proxmox_backup_write_data failed: {pbs_err_cstr:?}" - )); - } - } - - bytes_transferred += chunk_size; - } - - if proxmox_backup_close_image(pbs, pbs_device_id as u8, &mut pbs_err) < 0 { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!( - "proxmox_backup_close_image failed: {pbs_err_cstr:?}" - )); - } - } - } - - if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { - unsafe { - let pbs_err_cstr = CStr::from_ptr(pbs_err); - return Err(anyhow!("proxmox_backup_finish failed: {pbs_err_cstr:?}")); - } - } - - Ok(()) -} +mod vma2pbs; +use vma2pbs::backup_vma_to_pbs; fn main() -> Result<()> { let matches = command!() @@ -307,7 +86,8 @@ fn main() -> Result<()> { let compress = matches.get_flag("compress"); let encrypt = matches.get_flag("encrypt"); - let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string(); + let vma_file_path = matches.get_one::<String>("vma_file"); + let password_file = matches.get_one::<String>("password_file"); let pbs_password = match password_file { @@ -331,7 +111,7 @@ fn main() -> Result<()> { }; backup_vma_to_pbs( - vma_file_path, + vma_file_path.cloned(), pbs_repository, vmid, pbs_password, diff --git a/src/vma.rs b/src/vma.rs index 5ec3822..de3045d 100644 --- a/src/vma.rs +++ b/src/vma.rs @@ -2,10 +2,9 @@ extern crate anyhow; extern crate md5; use std::collections::HashMap; -use std::fs::File; -use std::io::{Read, Seek, SeekFrom}; +use std::io::Read; use std::mem::size_of; -use std::{cmp, str}; +use std::str; use anyhow::{anyhow, Result}; use bincode::Options; @@ -45,6 +44,8 @@ struct VmaHeader { reserved1: [u8; 4], #[serde(with = "BigArray")] pub dev_info: [VmaDeviceInfoHeader; VMA_MAX_DEVICES], + #[serde(skip_deserializing, skip_serializing)] + blob_buffer: Vec<u8>, } #[repr(C)] @@ -68,52 +69,35 @@ struct VmaExtentHeader { pub blockinfo: [VmaBlockInfo; VMA_BLOCKS_PER_EXTENT], } -#[derive(Clone)] -struct VmaBlockIndexEntry { - pub cluster_file_offset: u64, - pub mask: u16, -} - pub struct VmaReader { - vma_file: File, + vma_file: Box<dyn Read>, vma_header: VmaHeader, configs: HashMap<String, String>, - block_index: Vec<Vec<VmaBlockIndexEntry>>, - blocks_are_indexed: bool, } impl VmaReader { - pub fn new(vma_file_path: &str) -> Result<Self> { - let mut vma_file = match File::open(vma_file_path) { - Err(why) => return Err(anyhow!("couldn't open {}: {}", vma_file_path, why)), - Ok(file) => file, - }; - - let vma_header = Self::read_header(&mut vma_file).unwrap(); - let configs = Self::read_blob_buffer(&mut vma_file, &vma_header).unwrap(); - let block_index: Vec<Vec<VmaBlockIndexEntry>> = (0..256).map(|_| Vec::new()).collect(); + pub fn new(mut vma_file: impl Read + 'static) -> Result<Self> { + let vma_header = Self::read_header(&mut vma_file)?; + let configs = Self::read_configs(&vma_header)?; let instance = Self { - vma_file, + vma_file: Box::new(vma_file), vma_header, configs, - block_index, - blocks_are_indexed: false, }; Ok(instance) } - fn read_header(vma_file: &mut File) -> Result<VmaHeader> { - let mut buffer = Vec::with_capacity(size_of::<VmaHeader>()); - buffer.resize(size_of::<VmaHeader>(), 0); + fn read_header(vma_file: &mut impl Read) -> Result<VmaHeader> { + let mut buffer = vec![0; 12288]; vma_file.read_exact(&mut buffer)?; let bincode_options = bincode::DefaultOptions::new() .with_fixint_encoding() .with_big_endian(); - let vma_header: VmaHeader = bincode_options.deserialize(&buffer)?; + let mut vma_header: VmaHeader = bincode_options.deserialize(&buffer)?; if vma_header.magic != [b'V', b'M', b'A', 0] { return Err(anyhow!("Invalid magic number")); @@ -124,7 +108,7 @@ impl VmaReader { } buffer.resize(vma_header.header_size as usize, 0); - vma_file.read_exact(&mut buffer[size_of::<VmaHeader>()..])?; + vma_file.read_exact(&mut buffer[12288..])?; // Fill the MD5 sum field with zeros to compute the MD5 sum buffer[32..48].fill(0); @@ -134,89 +118,77 @@ impl VmaReader { return Err(anyhow!("Wrong VMA header checksum")); } - return Ok(vma_header); - } - - fn read_string_from_file(vma_file: &mut File, file_offset: u64) -> Result<String> { - let mut size_bytes = [0u8; 2]; - vma_file.seek(SeekFrom::Start(file_offset))?; - vma_file.read_exact(&mut size_bytes)?; - let size = u16::from_le_bytes(size_bytes) as usize; - let mut string_bytes = Vec::with_capacity(size - 1); - string_bytes.resize(size - 1, 0); - vma_file.read_exact(&mut string_bytes)?; - let string = str::from_utf8(&string_bytes)?; + let blob_buffer = &buffer[12288..vma_header.header_size as usize]; + vma_header.blob_buffer = blob_buffer.to_vec(); - return Ok(string.to_string()); + Ok(vma_header) } - fn read_blob_buffer( - vma_file: &mut File, - vma_header: &VmaHeader, - ) -> Result<HashMap<String, String>> { + fn read_configs(vma_header: &VmaHeader) -> Result<HashMap<String, String>> { let mut configs = HashMap::new(); for i in 0..VMA_MAX_CONFIGS { - let config_name_offset = vma_header.config_names[i]; - let config_data_offset = vma_header.config_data[i]; + let config_name_offset = vma_header.config_names[i] as usize; + let config_data_offset = vma_header.config_data[i] as usize; if config_name_offset == 0 || config_data_offset == 0 { continue; } - let config_name_file_offset = - (vma_header.blob_buffer_offset + config_name_offset) as u64; - let config_data_file_offset = - (vma_header.blob_buffer_offset + config_data_offset) as u64; - let config_name = Self::read_string_from_file(vma_file, config_name_file_offset)?; - let config_data = Self::read_string_from_file(vma_file, config_data_file_offset)?; - - configs.insert(String::from(config_name), String::from(config_data)); + let config_name_size_bytes = + vma_header.blob_buffer[config_name_offset..config_name_offset + 2].try_into()?; + let config_name_size = u16::from_le_bytes(config_name_size_bytes) as usize; + let config_name_bytes = &vma_header.blob_buffer + [config_name_offset + 2..config_name_offset + 1 + config_name_size]; + let config_name = str::from_utf8(config_name_bytes); + let config_data_size_bytes = + vma_header.blob_buffer[config_data_offset..config_data_offset + 2].try_into()?; + let config_data_size = u16::from_le_bytes(config_data_size_bytes) as usize; + let config_data_bytes = &vma_header.blob_buffer + [config_data_offset + 2..config_data_offset + 1 + config_data_size]; + let config_data = str::from_utf8(config_data_bytes); + + configs.insert(String::from(config_name?), String::from(config_data?)); } - return Ok(configs); + Ok(configs) } pub fn get_configs(&self) -> HashMap<String, String> { - return self.configs.clone(); + self.configs.clone() } - pub fn get_device_name(&mut self, device_id: usize) -> Option<String> { - if device_id >= VMA_MAX_DEVICES { - return None; - } - - let device_name_offset = self.vma_header.dev_info[device_id].device_name_offset; + pub fn get_device_name(&self, device_id: u8) -> Option<String> { + let device_name_offset = + self.vma_header.dev_info[device_id as usize].device_name_offset as usize; if device_name_offset == 0 { return None; } - let device_name_file_offset = - (self.vma_header.blob_buffer_offset + device_name_offset) as u64; - let device_name = - Self::read_string_from_file(&mut self.vma_file, device_name_file_offset).unwrap(); + let size_bytes = self.vma_header.blob_buffer[device_name_offset..device_name_offset + 2] + .try_into() + .unwrap(); + let size = u16::from_le_bytes(size_bytes) as usize; + let device_name_bytes = + &self.vma_header.blob_buffer[device_name_offset + 2..device_name_offset + 1 + size]; + let device_name = str::from_utf8(device_name_bytes); - return Some(device_name.to_string()); + Some(String::from(device_name.unwrap())) } - pub fn get_device_size(&self, device_id: usize) -> Option<u64> { - if device_id >= VMA_MAX_DEVICES { - return None; - } - - let dev_info = &self.vma_header.dev_info[device_id]; + pub fn get_device_size(&self, device_id: u8) -> Option<u64> { + let dev_info = &self.vma_header.dev_info[device_id as usize]; if dev_info.device_name_offset == 0 { return None; } - return Some(dev_info.device_size); + Some(dev_info.device_size) } - fn read_extent_header(vma_file: &mut File) -> Result<VmaExtentHeader> { - let mut buffer = Vec::with_capacity(size_of::<VmaExtentHeader>()); - buffer.resize(size_of::<VmaExtentHeader>(), 0); + fn read_extent_header(mut vma_file: impl Read) -> Result<VmaExtentHeader> { + let mut buffer = vec![0; size_of::<VmaExtentHeader>()]; vma_file.read_exact(&mut buffer)?; let bincode_options = bincode::DefaultOptions::new() @@ -237,113 +209,73 @@ impl VmaReader { return Err(anyhow!("Wrong VMA extent header checksum")); } - return Ok(vma_extent_header); + Ok(vma_extent_header) } - fn index_device_clusters(&mut self) -> Result<()> { - for device_id in 0..255 { - let device_size = match self.get_device_size(device_id) { - Some(x) => x, - None => { - continue; - } - }; + fn restore_extent<F>(&mut self, func: F) -> Result<()> + where + F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>, + { + let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; - let device_cluster_count = (device_size + 4096 * 16 - 1) / (4096 * 16); + for i in 0..VMA_BLOCKS_PER_EXTENT { + let blockinfo = &vma_extent_header.blockinfo[i]; - let block_index_entry_placeholder = VmaBlockIndexEntry { - cluster_file_offset: 0, - mask: 0, - }; - - self.block_index[device_id] - .resize(device_cluster_count as usize, block_index_entry_placeholder); - } + if blockinfo.dev_id == 0 { + continue; + } - let mut file_offset = self.vma_header.header_size as u64; - let vma_file_size = self.vma_file.metadata()?.len(); + let image_offset = 4096 * 16 * blockinfo.cluster_num as u64; + let is_zero = blockinfo.mask == 0; - while file_offset < vma_file_size { - self.vma_file.seek(SeekFrom::Start(file_offset))?; - let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; - file_offset += size_of::<VmaExtentHeader>() as u64; + let image_chunk_buffer = if is_zero { + None + } else { + let mut image_chunk_buffer = vec![0; 4096 * 16]; - for i in 0..VMA_BLOCKS_PER_EXTENT { - let blockinfo = &vma_extent_header.blockinfo[i]; + for j in 0..16 { + let block_mask = ((blockinfo.mask >> j) & 1) == 1; - if blockinfo.dev_id == 0 { - continue; + if block_mask { + self.vma_file.read_exact( + &mut image_chunk_buffer[4096 * j as usize..4096 * (j + 1) as usize], + )?; + } else { + image_chunk_buffer[4096 * j as usize..4096 * (j + 1) as usize].fill(0); + } } - let block_index_entry = VmaBlockIndexEntry { - cluster_file_offset: file_offset, - mask: blockinfo.mask, - }; + Some(image_chunk_buffer) + }; - self.block_index[blockinfo.dev_id as usize][blockinfo.cluster_num as usize] = - block_index_entry; - file_offset += blockinfo.mask.count_ones() as u64 * 4096; - } + func(blockinfo.dev_id, image_offset, image_chunk_buffer)?; } - self.blocks_are_indexed = true; - - return Ok(()); + Ok(()) } - pub fn read_device_contents( - &mut self, - device_id: usize, - buffer: &mut [u8], - offset: u64, - ) -> Result<bool> { - if device_id >= VMA_MAX_DEVICES { - return Err(anyhow!("invalid device id {}", device_id)); - } - - if offset % (4096 * 16) != 0 { - return Err(anyhow!("offset is not aligned to 65536")); - } - - // Make sure that the device clusters are already indexed - if !self.blocks_are_indexed { - self.index_device_clusters()?; - } - - let this_device_block_index = &self.block_index[device_id]; - let length = cmp::min( - buffer.len(), - this_device_block_index.len() * 4096 * 16 - offset as usize, - ); - let mut buffer_offset = 0; - let mut buffer_is_zero = true; - - while buffer_offset < length { - let block_index_entry = - &this_device_block_index[(offset as usize + buffer_offset) / (4096 * 16)]; - self.vma_file - .seek(SeekFrom::Start(block_index_entry.cluster_file_offset))?; - - for i in 0..16 { - if buffer_offset >= length { - break; - } - - let block_buffer_end = buffer_offset + cmp::min(length - buffer_offset, 4096); - let block_mask = ((block_index_entry.mask >> i) & 1) == 1; - - if block_mask { - self.vma_file - .read_exact(&mut buffer[buffer_offset..block_buffer_end])?; - buffer_is_zero = false; - } else { - buffer[buffer_offset..block_buffer_end].fill(0); - } - - buffer_offset += 4096; + pub fn restore<F>(&mut self, func: F) -> Result<()> + where + F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>, + { + loop { + match self.restore_extent(&func) { + Ok(()) => {} + Err(e) => match e.downcast_ref::<std::io::Error>() { + Some(ioerr) => { + if ioerr.kind() == std::io::ErrorKind::UnexpectedEof { + break; + } else { + return Err(anyhow!("Failed to read VMA file: {}", ioerr)); + } + } + _ => { + return Err(anyhow!("{}", e)); + } + }, } } - return Ok(buffer_is_zero); + Ok(()) } } diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs new file mode 100644 index 0000000..2451bff --- /dev/null +++ b/src/vma2pbs.rs @@ -0,0 +1,348 @@ +extern crate anyhow; +extern crate proxmox_backup_qemu; +extern crate scopeguard; + +use std::cell::RefCell; +use std::collections::HashMap; +use std::ffi::{c_char, CStr, CString}; +use std::fs::File; +use std::io::{stdin, BufRead, BufReader}; +use std::ptr; +use std::time::{SystemTime, UNIX_EPOCH}; + +use anyhow::{anyhow, Result}; +use proxmox_backup_qemu::*; +use scopeguard::defer; + +use crate::vma::*; + +const VMA_CLUSTER_SIZE: usize = 65536; + +pub fn backup_vma_to_pbs( + vma_file_path: Option<String>, + pbs_repository: String, + backup_id: String, + pbs_password: String, + keyfile: Option<String>, + key_password: Option<String>, + master_keyfile: Option<String>, + fingerprint: String, + compress: bool, + encrypt: bool, +) -> Result<()> { + println!("PBS repository: {}", pbs_repository); + println!("PBS fingerprint: {}", fingerprint); + println!("compress: {}", compress); + println!("encrypt: {}", encrypt); + + let backup_time = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); + println!("backup time: {}", backup_time); + + let mut pbs_err: *mut c_char = ptr::null_mut(); + + let pbs_repository_cstr = CString::new(pbs_repository)?; + let backup_id_cstr = CString::new(backup_id)?; + let pbs_password_cstr = CString::new(pbs_password)?; + let fingerprint_cstr = CString::new(fingerprint)?; + let keyfile_cstr = keyfile.map(|v| CString::new(v).unwrap()); + let keyfile_ptr = keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); + let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); + let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); + let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); + let master_keyfile_ptr = master_keyfile_cstr + .map(|v| v.as_ptr()) + .unwrap_or(ptr::null()); + + let pbs = proxmox_backup_new_ns( + pbs_repository_cstr.as_ptr(), + ptr::null(), + backup_id_cstr.as_ptr(), + backup_time, + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, + pbs_password_cstr.as_ptr(), + keyfile_ptr, + key_password_ptr, + master_keyfile_ptr, + true, + false, + fingerprint_cstr.as_ptr(), + &mut pbs_err, + ); + + defer! { + proxmox_backup_disconnect(pbs); + } + + if pbs.is_null() { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}")); + } + } + + let connect_result = proxmox_backup_connect(pbs, &mut pbs_err); + + if connect_result < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!("proxmox_backup_connect failed: {pbs_err_cstr:?}")); + } + } + + println!("Connected to Proxmox Backup Server"); + + let vma_file: Box<dyn BufRead> = match vma_file_path { + Some(vma_file_path) => match File::open(vma_file_path) { + Err(why) => return Err(anyhow!("Couldn't open file: {}", why)), + Ok(file) => Box::new(BufReader::new(file)), + }, + None => Box::new(BufReader::new(stdin())), + }; + let mut vma_reader = VmaReader::new(vma_file)?; + + let start_transfer_time = SystemTime::now(); + + // Handle configs + let configs = vma_reader.get_configs(); + for (config_name, config_data) in configs { + println!("CFG: size: {} name: {}", config_data.len(), config_name); + + let config_name_cstr = CString::new(config_name)?; + + if proxmox_backup_add_config( + pbs, + config_name_cstr.as_ptr(), + config_data.as_ptr(), + config_data.len() as u64, + &mut pbs_err, + ) < 0 + { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_add_config failed: {pbs_err_cstr:?}" + )); + } + } + } + + let mut pbs_device_id_map = [-1i32; 256]; + let mut device_sizes = [0u64; 256]; + + // Handle block devices + for device_id in 0..255 { + let device_name = match vma_reader.get_device_name(device_id) { + Some(x) => x, + None => continue, + }; + + let device_size = match vma_reader.get_device_size(device_id) { + Some(x) => x, + None => continue, + }; + + device_sizes[device_id as usize] = device_size; + + println!( + "DEV: dev_id={} size: {} devname: {}", + device_id, device_size, device_name + ); + + let device_name_cstr = CString::new(device_name)?; + let pbs_device_id = proxmox_backup_register_image( + pbs, + device_name_cstr.as_ptr(), + device_size, + false, + &mut pbs_err, + ); + + if pbs_device_id < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_register_image failed: {pbs_err_cstr:?}" + )); + } + } + + pbs_device_id_map[device_id as usize] = pbs_device_id; + } + + struct ImageChunk { + sub_chunks: HashMap<u8, Option<Vec<u8>>>, + mask: u64, + non_zero_mask: u64, + } + + let images_chunks: RefCell<HashMap<u8, HashMap<u64, ImageChunk>>> = + RefCell::new(HashMap::new()); + + vma_reader.restore(|dev_id, offset, buffer| { + let pbs_device_id = pbs_device_id_map[dev_id as usize]; + + if pbs_device_id < 0 { + return Err(anyhow!( + "Reference to unknown device id {} in VMA file", + dev_id + )); + } + + let mut images_chunks = images_chunks.borrow_mut(); + let image_chunks = images_chunks.get_mut(&dev_id); + let pbs_chunk_offset = + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE * (offset / PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE); + let sub_chunk_index = + ((offset % PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE) / VMA_CLUSTER_SIZE as u64) as u8; + + let pbs_chunk_size = + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE.min(device_sizes[dev_id as usize] - pbs_chunk_offset); + + let pbs_upload_chunk = |pbs_chunk_buffer: Option<&[u8]>| { + println!( + "Uploading dev_id: {} offset: {:#0X} - {:#0X}", + dev_id, + pbs_chunk_offset, + pbs_chunk_offset + pbs_chunk_size, + ); + + let mut pbs_err: *mut c_char = ptr::null_mut(); + + let write_data_result = proxmox_backup_write_data( + pbs, + pbs_device_id as u8, + match pbs_chunk_buffer { + Some(pbs_chunk_buffer) => pbs_chunk_buffer.as_ptr(), + None => ptr::null(), + }, + pbs_chunk_offset, + pbs_chunk_size, + &mut pbs_err, + ); + + if write_data_result < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_write_data failed: {pbs_err_cstr:?}" + )); + } + } + + Ok(()) + }; + + let insert_image_chunk = |image_chunks: &mut HashMap<u64, ImageChunk>, + buffer: Option<Vec<u8>>| { + let mut sub_chunks: HashMap<u8, Option<Vec<u8>>> = HashMap::new(); + let mask = 1 << sub_chunk_index; + let non_zero_mask = buffer.is_some() as u64; + sub_chunks.insert(sub_chunk_index, buffer); + + let image_chunk = ImageChunk { + sub_chunks, + mask, + non_zero_mask, + }; + + image_chunks.insert(pbs_chunk_offset, image_chunk); + }; + + match image_chunks { + Some(image_chunks) => { + let image_chunk = image_chunks.get_mut(&pbs_chunk_offset); + + match image_chunk { + Some(image_chunk) => { + image_chunk.mask |= 1 << sub_chunk_index; + image_chunk.non_zero_mask |= (buffer.is_some() as u64) << sub_chunk_index; + image_chunk.sub_chunks.insert(sub_chunk_index, buffer); + + let sub_chunk_count = + ((pbs_chunk_size + 65535) / VMA_CLUSTER_SIZE as u64) as u8; + let pbs_chunk_mask = 1_u64 + .checked_shl(sub_chunk_count.into()) + .unwrap_or(0) + .wrapping_sub(1); + + if image_chunk.mask == pbs_chunk_mask { + if image_chunk.non_zero_mask == 0 { + pbs_upload_chunk(None)?; + } else { + let mut pbs_chunk_buffer = + proxmox_io::boxed::zeroed(pbs_chunk_size as usize); + + for i in 0..sub_chunk_count { + let sub_chunk = image_chunk.sub_chunks.get(&i).unwrap(); + let a = i as usize * VMA_CLUSTER_SIZE; + let b = (a + VMA_CLUSTER_SIZE).min(pbs_chunk_size as usize); + + match sub_chunk { + Some(sub_chunk) => { + pbs_chunk_buffer[a..b] + .copy_from_slice(&sub_chunk[0..b - a]); + } + None => pbs_chunk_buffer[a..b].fill(0), + } + } + + pbs_upload_chunk(Some(&*pbs_chunk_buffer))?; + } + + image_chunks.remove(&pbs_chunk_offset); + } + } + None => { + if pbs_chunk_size <= VMA_CLUSTER_SIZE as u64 { + pbs_upload_chunk(buffer.as_deref())?; + } else { + insert_image_chunk(image_chunks, buffer); + } + } + } + } + None => { + if pbs_chunk_size <= VMA_CLUSTER_SIZE as u64 { + pbs_upload_chunk(buffer.as_deref())?; + } else { + let mut image_chunks: HashMap<u64, ImageChunk> = HashMap::new(); + insert_image_chunk(&mut image_chunks, buffer); + images_chunks.insert(dev_id, image_chunks); + } + } + } + + Ok(()) + })?; + + for pbs_device_id in pbs_device_id_map.iter().take(255) { + if *pbs_device_id < 0 { + continue; + } + + if proxmox_backup_close_image(pbs, *pbs_device_id as u8, &mut pbs_err) < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!( + "proxmox_backup_close_image failed: {pbs_err_cstr:?}" + )); + } + } + } + + if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { + unsafe { + let pbs_err_cstr = CStr::from_ptr(pbs_err); + return Err(anyhow!("proxmox_backup_finish failed: {pbs_err_cstr:?}")); + } + } + + println!( + "Backup finished within {}ms", + SystemTime::now() + .duration_since(start_transfer_time)? + .as_millis() + ); + + Ok(()) +} -- 2.39.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin Filip Schauer @ 2024-03-06 15:49 ` Max Carrara 2024-03-12 14:04 ` Filip Schauer 0 siblings, 1 reply; 14+ messages in thread From: Max Carrara @ 2024-03-06 15:49 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Filip Schauer On 3/5/24 14:56, Filip Schauer wrote: > This allows the user to stream a compressed VMA file directly to a PBS > without the need to extract it to an intermediate .vma file. > > Example usage: > > zstd -d --stdout vzdump.vma.zstd | vma-to-pbs \ > --repository <auth_id@host:port:datastore> \ > --vmid 123 \ > --password_file pbs_password > > Signed-off-by: Filip Schauer <f.schauer@proxmox.com> > --- > src/main.rs | 232 +-------------------------------- > src/vma.rs | 270 ++++++++++++++------------------------ > src/vma2pbs.rs | 348 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 455 insertions(+), 395 deletions(-) > create mode 100644 src/vma2pbs.rs > > diff --git a/src/main.rs b/src/main.rs > index b58387b..dc8171e 100644 > --- a/src/main.rs > +++ b/src/main.rs > @@ -1,235 +1,14 @@ > extern crate anyhow; > extern crate clap; > -extern crate proxmox_backup_qemu; > -extern crate proxmox_io; > extern crate proxmox_sys; > -extern crate scopeguard; As mentioned in my previous comments, the remaining `extern crate` statements here are no longer necessary since edition 2018 [0]. > > -use std::env; > -use std::ffi::{c_char, CStr, CString}; > -use std::ptr; > -use std::time::{SystemTime, UNIX_EPOCH}; > - > -use anyhow::{anyhow, Context, Result}; > +use anyhow::Result; > use clap::{command, Arg, ArgAction}; > -use proxmox_backup_qemu::*; > use proxmox_sys::linux::tty; > -use scopeguard::defer; > > mod vma; > -use vma::*; > - > -fn backup_vma_to_pbs( > - vma_file_path: String, > - pbs_repository: String, > - backup_id: String, > - pbs_password: String, > - keyfile: Option<String>, > - key_password: Option<String>, > - master_keyfile: Option<String>, > - fingerprint: String, > - compress: bool, > - encrypt: bool, > -) -> Result<()> { > - println!("VMA input file: {}", vma_file_path); > - println!("PBS repository: {}", pbs_repository); > - println!("PBS fingerprint: {}", fingerprint); > - println!("compress: {}", compress); > - println!("encrypt: {}", encrypt); > - > - let backup_time = SystemTime::now() > - .duration_since(UNIX_EPOCH) > - .unwrap() > - .as_secs(); > - println!("backup time: {}", backup_time); > - > - let mut pbs_err: *mut c_char = ptr::null_mut(); > - > - let pbs_repository_cstr = CString::new(pbs_repository).unwrap(); > - let backup_id_cstr = CString::new(backup_id).unwrap(); > - let pbs_password_cstr = CString::new(pbs_password).unwrap(); > - let fingerprint_cstr = CString::new(fingerprint).unwrap(); > - let keyfile_cstr = keyfile.map(|v| CString::new(v).unwrap()); > - let keyfile_ptr = keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > - let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); > - let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > - let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); > - let master_keyfile_ptr = master_keyfile_cstr > - .map(|v| v.as_ptr()) > - .unwrap_or(ptr::null()); > - > - let pbs = proxmox_backup_new_ns( > - pbs_repository_cstr.as_ptr(), > - ptr::null(), > - backup_id_cstr.as_ptr(), > - backup_time, > - PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, > - pbs_password_cstr.as_ptr(), > - keyfile_ptr, > - key_password_ptr, > - master_keyfile_ptr, > - true, > - false, > - fingerprint_cstr.as_ptr(), > - &mut pbs_err, > - ); > - > - defer! { > - proxmox_backup_disconnect(pbs); > - } > - > - if pbs == ptr::null_mut() { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}")); > - } > - } > - > - let connect_result = proxmox_backup_connect(pbs, &mut pbs_err); > - > - if connect_result < 0 { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!("proxmox_backup_connect failed: {pbs_err_cstr:?}")); > - } > - } > - > - let mut vma_reader = VmaReader::new(&vma_file_path)?; > - > - // Handle configs > - let configs = vma_reader.get_configs(); > - for (config_name, config_data) in configs { > - println!("CFG: size: {} name: {}", config_data.len(), config_name); > - > - let config_name_cstr = CString::new(config_name).unwrap(); > - > - if proxmox_backup_add_config( > - pbs, > - config_name_cstr.as_ptr(), > - config_data.as_ptr(), > - config_data.len() as u64, > - &mut pbs_err, > - ) < 0 > - { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!( > - "proxmox_backup_add_config failed: {pbs_err_cstr:?}" > - )); > - } > - } > - } > - > - // Handle block devices > - for device_id in 0..255 { > - let device_name = match vma_reader.get_device_name(device_id) { > - Some(x) => x, > - None => { > - continue; > - } > - }; > - > - let device_size = match vma_reader.get_device_size(device_id) { > - Some(x) => x, > - None => { > - continue; > - } > - }; > - > - println!( > - "DEV: dev_id={} size: {} devname: {}", > - device_id, device_size, device_name > - ); > - > - let device_name_cstr = CString::new(device_name).unwrap(); > - let pbs_device_id = proxmox_backup_register_image( > - pbs, > - device_name_cstr.as_ptr(), > - device_size, > - false, > - &mut pbs_err, > - ); > - > - if pbs_device_id < 0 { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!( > - "proxmox_backup_register_image failed: {pbs_err_cstr:?}" > - )); > - } > - } > - > - let mut image_chunk_buffer = > - proxmox_io::boxed::zeroed(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE as usize); > - let mut bytes_transferred = 0; > - > - while bytes_transferred < device_size { > - let bytes_left = device_size - bytes_transferred; > - let chunk_size = bytes_left.min(PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE); > - println!( > - "Uploading dev_id: {} offset: {:#0X} - {:#0X}", > - device_id, > - bytes_transferred, > - bytes_transferred + chunk_size > - ); > - > - let is_zero_chunk = vma_reader > - .read_device_contents( > - device_id, > - &mut image_chunk_buffer[0..chunk_size as usize], > - bytes_transferred, > - ) > - .with_context(|| { > - format!( > - "read {} bytes at offset {} from disk {} from VMA file", > - chunk_size, bytes_transferred, device_id > - ) > - })?; > - > - let write_data_result = proxmox_backup_write_data( > - pbs, > - pbs_device_id as u8, > - if is_zero_chunk { > - ptr::null() > - } else { > - image_chunk_buffer.as_ptr() > - }, > - bytes_transferred, > - chunk_size, > - &mut pbs_err, > - ); > - > - if write_data_result < 0 { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!( > - "proxmox_backup_write_data failed: {pbs_err_cstr:?}" > - )); > - } > - } > - > - bytes_transferred += chunk_size; > - } > - > - if proxmox_backup_close_image(pbs, pbs_device_id as u8, &mut pbs_err) < 0 { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!( > - "proxmox_backup_close_image failed: {pbs_err_cstr:?}" > - )); > - } > - } > - } > - > - if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { > - unsafe { > - let pbs_err_cstr = CStr::from_ptr(pbs_err); > - return Err(anyhow!("proxmox_backup_finish failed: {pbs_err_cstr:?}")); > - } > - } > - > - Ok(()) > -} > +mod vma2pbs; > +use vma2pbs::backup_vma_to_pbs; > > fn main() -> Result<()> { > let matches = command!() > @@ -307,7 +86,8 @@ fn main() -> Result<()> { > let compress = matches.get_flag("compress"); > let encrypt = matches.get_flag("encrypt"); > > - let vma_file_path = matches.get_one::<String>("vma_file").unwrap().to_string(); > + let vma_file_path = matches.get_one::<String>("vma_file"); You got rid of the `unwrap()`, good! > + > let password_file = matches.get_one::<String>("password_file"); > > let pbs_password = match password_file { > @@ -331,7 +111,7 @@ fn main() -> Result<()> { > }; > > backup_vma_to_pbs( > - vma_file_path, > + vma_file_path.cloned(), > pbs_repository, > vmid, > pbs_password, > diff --git a/src/vma.rs b/src/vma.rs > index 5ec3822..de3045d 100644 > --- a/src/vma.rs > +++ b/src/vma.rs > @@ -2,10 +2,9 @@ extern crate anyhow; > extern crate md5; > > use std::collections::HashMap; > -use std::fs::File; > -use std::io::{Read, Seek, SeekFrom}; > +use std::io::Read; > use std::mem::size_of; > -use std::{cmp, str}; > +use std::str; > > use anyhow::{anyhow, Result}; > use bincode::Options; > @@ -45,6 +44,8 @@ struct VmaHeader { > reserved1: [u8; 4], > #[serde(with = "BigArray")] > pub dev_info: [VmaDeviceInfoHeader; VMA_MAX_DEVICES], > + #[serde(skip_deserializing, skip_serializing)] > + blob_buffer: Vec<u8>, > } > > #[repr(C)] > @@ -68,52 +69,35 @@ struct VmaExtentHeader { > pub blockinfo: [VmaBlockInfo; VMA_BLOCKS_PER_EXTENT], > } > > -#[derive(Clone)] > -struct VmaBlockIndexEntry { > - pub cluster_file_offset: u64, > - pub mask: u16, > -} > - > pub struct VmaReader { > - vma_file: File, > + vma_file: Box<dyn Read>, > vma_header: VmaHeader, > configs: HashMap<String, String>, > - block_index: Vec<Vec<VmaBlockIndexEntry>>, > - blocks_are_indexed: bool, > } > > impl VmaReader { > - pub fn new(vma_file_path: &str) -> Result<Self> { > - let mut vma_file = match File::open(vma_file_path) { > - Err(why) => return Err(anyhow!("couldn't open {}: {}", vma_file_path, why)), > - Ok(file) => file, > - }; > - > - let vma_header = Self::read_header(&mut vma_file).unwrap(); > - let configs = Self::read_blob_buffer(&mut vma_file, &vma_header).unwrap(); > - let block_index: Vec<Vec<VmaBlockIndexEntry>> = (0..256).map(|_| Vec::new()).collect(); > + pub fn new(mut vma_file: impl Read + 'static) -> Result<Self> { Why use `impl Read + 'static`? This makes the function implicitly generic. That can sometimes be totally fine, but is there any benefit you get from monomorphization in this case? Furthermore, `VmaReader::vma_file` is `Box<dyn Read>` - why have that one dynamically dispatched, and the other one not? > + let vma_header = Self::read_header(&mut vma_file)?; > + let configs = Self::read_configs(&vma_header)?; > > let instance = Self { > - vma_file, > + vma_file: Box::new(vma_file), ... especially if you `Box` it here anyway. > vma_header, > configs, > - block_index, > - blocks_are_indexed: false, > }; > > Ok(instance) > } > > - fn read_header(vma_file: &mut File) -> Result<VmaHeader> { > - let mut buffer = Vec::with_capacity(size_of::<VmaHeader>()); > - buffer.resize(size_of::<VmaHeader>(), 0); > + fn read_header(vma_file: &mut impl Read) -> Result<VmaHeader> { Same as above here - is there any benefit from being generic over `Read` here? > + let mut buffer = vec![0; 12288]; What is this size? Should be a constant. > vma_file.read_exact(&mut buffer)?; > > let bincode_options = bincode::DefaultOptions::new() > .with_fixint_encoding() > .with_big_endian(); > > - let vma_header: VmaHeader = bincode_options.deserialize(&buffer)?; > + let mut vma_header: VmaHeader = bincode_options.deserialize(&buffer)?; > > if vma_header.magic != [b'V', b'M', b'A', 0] { As I commented on a previous patch, this array should also be a constant. > return Err(anyhow!("Invalid magic number")); Use `anyhow::bail`. [4] > @@ -124,7 +108,7 @@ impl VmaReader { > } > > buffer.resize(vma_header.header_size as usize, 0); > - vma_file.read_exact(&mut buffer[size_of::<VmaHeader>()..])?; > + vma_file.read_exact(&mut buffer[12288..])?; Constant? ;) > > // Fill the MD5 sum field with zeros to compute the MD5 sum > buffer[32..48].fill(0); > @@ -134,89 +118,77 @@ impl VmaReader { > return Err(anyhow!("Wrong VMA header checksum")); > }> > - return Ok(vma_header); > - } > - > - fn read_string_from_file(vma_file: &mut File, file_offset: u64) -> Result<String> { > - let mut size_bytes = [0u8; 2]; > - vma_file.seek(SeekFrom::Start(file_offset))?; > - vma_file.read_exact(&mut size_bytes)?; > - let size = u16::from_le_bytes(size_bytes) as usize; > - let mut string_bytes = Vec::with_capacity(size - 1); > - string_bytes.resize(size - 1, 0); > - vma_file.read_exact(&mut string_bytes)?; > - let string = str::from_utf8(&string_bytes)?; > + let blob_buffer = &buffer[12288..vma_header.header_size as usize]; Use a constant here too, please. > + vma_header.blob_buffer = blob_buffer.to_vec(); > > - return Ok(string.to_string()); > + Ok(vma_header) > } > > - fn read_blob_buffer( > - vma_file: &mut File, > - vma_header: &VmaHeader, > - ) -> Result<HashMap<String, String>> { > + fn read_configs(vma_header: &VmaHeader) -> Result<HashMap<String, String>> { I assume config values are key-value pairs. Would it make sense to encapsulate this map in a separate struct to make the code more expressive? > let mut configs = HashMap::new(); > > for i in 0..VMA_MAX_CONFIGS { > - let config_name_offset = vma_header.config_names[i]; > - let config_data_offset = vma_header.config_data[i]; > + let config_name_offset = vma_header.config_names[i] as usize; > + let config_data_offset = vma_header.config_data[i] as usize; > > if config_name_offset == 0 || config_data_offset == 0 { > continue; > } > > - let config_name_file_offset = > - (vma_header.blob_buffer_offset + config_name_offset) as u64; > - let config_data_file_offset = > - (vma_header.blob_buffer_offset + config_data_offset) as u64; > - let config_name = Self::read_string_from_file(vma_file, config_name_file_offset)?; > - let config_data = Self::read_string_from_file(vma_file, config_data_file_offset)?; > - > - configs.insert(String::from(config_name), String::from(config_data)); The code starting here ... > + let config_name_size_bytes = > + vma_header.blob_buffer[config_name_offset..config_name_offset + 2].try_into()?; > + let config_name_size = u16::from_le_bytes(config_name_size_bytes) as usize; > + let config_name_bytes = &vma_header.blob_buffer > + [config_name_offset + 2..config_name_offset + 1 + config_name_size]; > + let config_name = str::from_utf8(config_name_bytes); ... and ending here looks suspiciously similar .. .. to the code starting here ... > + let config_data_size_bytes = > + vma_header.blob_buffer[config_data_offset..config_data_offset + 2].try_into()?; > + let config_data_size = u16::from_le_bytes(config_data_size_bytes) as usize; > + let config_data_bytes = &vma_header.blob_buffer > + [config_data_offset + 2..config_data_offset + 1 + config_data_size]; > + let config_data = str::from_utf8(config_data_bytes); ... and ending here. Should be a helper function with a docstring, IMO. Especially since there's a lot of stuff going on - the indexing of the buffers in particular could use a description. > + > + configs.insert(String::from(config_name?), String::from(config_data?)); Why `?` here on the variable names themselves? Why not earlier? Are these names always UTF-8 encoded? > } > > - return Ok(configs); > + Ok(configs) > } > > pub fn get_configs(&self) -> HashMap<String, String> { > - return self.configs.clone(); > + self.configs.clone() > } > > - pub fn get_device_name(&mut self, device_id: usize) -> Option<String> { > - if device_id >= VMA_MAX_DEVICES { > - return None; > - } > - > - let device_name_offset = self.vma_header.dev_info[device_id].device_name_offset; > + pub fn get_device_name(&self, device_id: u8) -> Option<String> { Could use a `type VmaDeviceId = u8;` declared at the top of the file here. > + let device_name_offset = > + self.vma_header.dev_info[device_id as usize].device_name_offset as usize; > > if device_name_offset == 0 { > return None; > } > > - let device_name_file_offset = > - (self.vma_header.blob_buffer_offset + device_name_offset) as u64; > - let device_name = > - Self::read_string_from_file(&mut self.vma_file, device_name_file_offset).unwrap(); > + let size_bytes = self.vma_header.blob_buffer[device_name_offset..device_name_offset + 2] > + .try_into() > + .unwrap(); Is it safe to `unwrap()` here? If yes, add a comment why, if not, add `anyhow::Context` and abort early with `?` otherwise. > + let size = u16::from_le_bytes(size_bytes) as usize; > + let device_name_bytes = > + &self.vma_header.blob_buffer[device_name_offset + 2..device_name_offset + 1 + size]; > + let device_name = str::from_utf8(device_name_bytes); > > - return Some(device_name.to_string()); > + Some(String::from(device_name.unwrap())) Is it safe to `unwrap()` here too? Same as above. Are device names always encoded in UTF-8? Also, this code looks suspiciously similar to the two blocks I marked above, so could probably use the same function here too. > } > > - pub fn get_device_size(&self, device_id: usize) -> Option<u64> { > - if device_id >= VMA_MAX_DEVICES { > - return None; > - } > - > - let dev_info = &self.vma_header.dev_info[device_id]; > + pub fn get_device_size(&self, device_id: u8) -> Option<u64> { `type VmaDeviceId = u8;` - same as above. Using `type VmaDeviceSize = u64;` for the return value would also be nice. > + let dev_info = &self.vma_header.dev_info[device_id as usize]; > > if dev_info.device_name_offset == 0 { > return None; > } > > - return Some(dev_info.device_size); > + Some(dev_info.device_size) > } > > - fn read_extent_header(vma_file: &mut File) -> Result<VmaExtentHeader> { > - let mut buffer = Vec::with_capacity(size_of::<VmaExtentHeader>()); > - buffer.resize(size_of::<VmaExtentHeader>(), 0); > + fn read_extent_header(mut vma_file: impl Read) -> Result<VmaExtentHeader> { Same as I mentioned before - is there any benefit to being generic here? > + let mut buffer = vec![0; size_of::<VmaExtentHeader>()]; > vma_file.read_exact(&mut buffer)?; > > let bincode_options = bincode::DefaultOptions::new() > @@ -237,113 +209,73 @@ impl VmaReader { > return Err(anyhow!("Wrong VMA extent header checksum")); > } > > - return Ok(vma_extent_header); > + Ok(vma_extent_header) > } > > - fn index_device_clusters(&mut self) -> Result<()> { > - for device_id in 0..255 { > - let device_size = match self.get_device_size(device_id) { > - Some(x) => x, > - None => { > - continue; > - } > - }; > + fn restore_extent<F>(&mut self, func: F) -> Result<()> > + where > + F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>, > + { A couple of things: What is `func` what what does it do? Should be described in a docstring. Regarding `F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>`: * Why `Fn` and not `FnOnce`? You call this with a closure later on. * What does the `u8` stand for? * What does the `u64` stand for? * What does the `Option<Vec<u8>>` stand for? You might benefit from type aliases here, so the signature reveals a little more about what`func` should actually be. > + let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; > > - let device_cluster_count = (device_size + 4096 * 16 - 1) / (4096 * 16); > + for i in 0..VMA_BLOCKS_PER_EXTENT { > + let blockinfo = &vma_extent_header.blockinfo[i]; > > - let block_index_entry_placeholder = VmaBlockIndexEntry { > - cluster_file_offset: 0, > - mask: 0, > - }; > - > - self.block_index[device_id] > - .resize(device_cluster_count as usize, block_index_entry_placeholder); > - } > + if blockinfo.dev_id == 0 { > + continue; > + } > > - let mut file_offset = self.vma_header.header_size as u64; > - let vma_file_size = self.vma_file.metadata()?.len(); > + let image_offset = 4096 * 16 * blockinfo.cluster_num as u64; Should use constants here. > + let is_zero = blockinfo.mask == 0; I'm usually in favour of assigning the result of conditional checks to variables first, but is that really necessary here? > > - while file_offset < vma_file_size { > - self.vma_file.seek(SeekFrom::Start(file_offset))?; > - let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; > - file_offset += size_of::<VmaExtentHeader>() as u64; > + let image_chunk_buffer = if is_zero { It's only used here after all, and inlining it wouldn't make the code more complex at all. > + None > + } else { > + let mut image_chunk_buffer = vec![0; 4096 * 16]; Again, should use constants here. > > - for i in 0..VMA_BLOCKS_PER_EXTENT { > - let blockinfo = &vma_extent_header.blockinfo[i]; > + for j in 0..16 { What does "j" stand for? > + let block_mask = ((blockinfo.mask >> j) & 1) == 1; What does this `block_mask` mean exactly? It's a `bool`, so why not name it `does_match_mask`? > > - if blockinfo.dev_id == 0 { > - continue; > + if block_mask { > + self.vma_file.read_exact( > + &mut image_chunk_buffer[4096 * j as usize..4096 * (j + 1) as usize], Why does `image_chunk_buffer` need to be indexed the way it does? Could this perhaps be expressed better via an explicit `Range` that you define as a variable first? Or maybe define the bounds of the range instead first? E.g.: let start = 4096 * j as usize; let end = 4096 * (j + 1) as usize; > + )?; > + } else { > + image_chunk_buffer[4096 * j as usize..4096 * (j + 1) as usize].fill(0); You use the same range here as well. > + } > } > > - let block_index_entry = VmaBlockIndexEntry { > - cluster_file_offset: file_offset, > - mask: blockinfo.mask, > - }; > + Some(image_chunk_buffer) > + }; > > - self.block_index[blockinfo.dev_id as usize][blockinfo.cluster_num as usize] = > - block_index_entry; > - file_offset += blockinfo.mask.count_ones() as u64 * 4096; > - } > + func(blockinfo.dev_id, image_offset, image_chunk_buffer)?; So, `func` gets called here at the end, it gets a device ID, an offset, and a buffer. My first guess is that it's used to actually perform the restoring / conversion of the chunks - so why not make that clear from the beginning? Also, coming back to `F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>`: Why does the buffer have to be wrapped in an `Option`? Why not just use an empty buffer to represent that nothing was passed on? Also, `Vec<u8>` could probably be `&[u8]`, couldn't it? > } > > - self.blocks_are_indexed = true; > - > - return Ok(()); > + Ok(()) > } > > - pub fn read_device_contents( > - &mut self, > - device_id: usize, > - buffer: &mut [u8], > - offset: u64, > - ) -> Result<bool> { > - if device_id >= VMA_MAX_DEVICES { > - return Err(anyhow!("invalid device id {}", device_id)); > - } > - > - if offset % (4096 * 16) != 0 { > - return Err(anyhow!("offset is not aligned to 65536")); > - } > - > - // Make sure that the device clusters are already indexed > - if !self.blocks_are_indexed { > - self.index_device_clusters()?; > - } > - > - let this_device_block_index = &self.block_index[device_id]; > - let length = cmp::min( > - buffer.len(), > - this_device_block_index.len() * 4096 * 16 - offset as usize, > - ); > - let mut buffer_offset = 0; > - let mut buffer_is_zero = true; > - > - while buffer_offset < length { > - let block_index_entry = > - &this_device_block_index[(offset as usize + buffer_offset) / (4096 * 16)]; > - self.vma_file > - .seek(SeekFrom::Start(block_index_entry.cluster_file_offset))?; > - > - for i in 0..16 { > - if buffer_offset >= length { > - break; > - } > - > - let block_buffer_end = buffer_offset + cmp::min(length - buffer_offset, 4096); > - let block_mask = ((block_index_entry.mask >> i) & 1) == 1; > - > - if block_mask { > - self.vma_file > - .read_exact(&mut buffer[buffer_offset..block_buffer_end])?; > - buffer_is_zero = false; > - } else { > - buffer[buffer_offset..block_buffer_end].fill(0); > - } > - > - buffer_offset += 4096; > + pub fn restore<F>(&mut self, func: F) -> Result<()> > + where > + F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>, > + { `func` returns here yet again. There are some more notes below. > + loop { > + match self.restore_extent(&func) { > + Ok(()) => {} > + Err(e) => match e.downcast_ref::<std::io::Error>() { > + Some(ioerr) => { > + if ioerr.kind() == std::io::ErrorKind::UnexpectedEof { > + break; Why is it fine to `break` on that error explicitly? Could use a comment, perhaps. > + } else { > + return Err(anyhow!("Failed to read VMA file: {}", ioerr)); Use `anyhow::format_err` instead of `anyhow::anyhow`, as we use the former by convention. Futhermore, to really levearge `anyhow` here, wrap the original error and provide some `anyhow::Context`: return Err(format_err!(ioerr)).context("failed to read VMA file"); > + } > + } > + _ => { > + return Err(anyhow!("{}", e)); Same as above. > + } > + }, > } > } > > - return Ok(buffer_is_zero); > + Ok(()) > } > } Some more thoughts `func` and `F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>`: Unless it makes sense to be generic in this context, it's better to define a type alias for the function itself (and use some more aliases for the other types), like so: type DeviceId = u8; type ImageOffset = u64; type RestoreHandler = Box<dyn Fn(DeviceId, ImageOffset, &[u8]) -> Result<()>>; That is much more expressive and also allows you to get rid of the generic. Putting it in a `Box` is less overhead than you think, because you usually need to to that once anyway. You can still use freestanding functions instead of closures, too: fn do_the_restoration(id: DeviceId, offset: ImageOffset, buf: &[u8]) -> Result<()> { todo!() } // ... let handler: RestoreHandler = Box::new(do_the_restoration); Since `RestoreHandler` is constrained to `Fn`, you can then pass it by reference: pub fn restore(&mut self, handler: &RestoreHandler) -> Result<()> { // ... } > diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs > new file mode 100644 > index 0000000..2451bff > --- /dev/null > +++ b/src/vma2pbs.rs > @@ -0,0 +1,348 @@ > +extern crate anyhow; > +extern crate proxmox_backup_qemu; > +extern crate scopeguard; > + > +use std::cell::RefCell; > +use std::collections::HashMap; > +use std::ffi::{c_char, CStr, CString}; > +use std::fs::File; > +use std::io::{stdin, BufRead, BufReader}; > +use std::ptr; > +use std::time::{SystemTime, UNIX_EPOCH}; > + > +use anyhow::{anyhow, Result}; > +use proxmox_backup_qemu::*; Please refrain from using glob imports; while they're convenient, they can sometimes lead to name collisions. I know that's not the case here, but it's still something to keep in mind. One good use of glob imports would be e.g. importing a so-called "prelude". I'm sure you've heard of it before, but I'd still like to explain it here. A prelude makes it easier to import many common traits (and types) all at once, because in one way or another, you'll end up using them anyway. A good example of this is `std::io::prelude`. [1] There's also an implicit prelude that's automatically used everywhere. [2] > +use scopeguard::defer; > + > +use crate::vma::*; Same here. > + > +const VMA_CLUSTER_SIZE: usize = 65536; > + As mentioned in my comments regarding patch 1, there are several things about the function below that I'd like to note: 1. The function has way too many arguments. `cargo clippy` would emit a warning here, but unfortunately I wasn't able to actually build the project (see my reply to your cover letter). Instead, (as I mentioned before) this function should take a struct that represents its arguments, as that makes it vastly more readable and also makes it much easier to add additional parameters later on if necessary. It's even better if you implement some builder pattern around it - e.g. *required* arguments need to be put into `::new()`, additional arguments / flags can be initialized with `::default()` and specified via setters if necessary. Note that you don't have to go all-in on this - a simple struct usually is enough. Even if it seems boiler-platey, it's going to save you and others a lot of time refactoring, should this function's signature ever change (from what I've experienced, these things *do* eventually change sooner or later in our codebase, in general). I also realize we have such functions floating around in other places, but IMHO there's no need to continue that pattern. 2. The body of this function really needs to be broken up into seveal smaller pieces. There are some things that will look jarring either way (see inline comments) but to make reading, debugging, error handling and many other things easier, something like this should always be broken up into smaller parts IMHO. I'd like to note that we have giant functions like this in other places, but I personally think that we shouldn't continue going down that route any further (but that's a discussion for another time). See comments inline for things that could maybe be put into separate functions. 3. As the function is now `pub`, it implicitly defines an API (boundary). You should therefore take several things into special consideration: a. "Will others use that function outside of this crate?" If the answer is "no", then you should mark it as `pub(crate)`. This also makes the following points less important, but you should still consider them regardless. b. "How can I guarantee that this function's signature will remain stable?" This is why I suggested to use an arg-struct earlier - should a new `Option`al parameter be added, call sites won't have to change. Otherwise, you'd need to explicitly pass on `None` everywhere the function is used. If the function's signature is less likely to change, it's also more friendly for semver [3]. c. "Is it necessary to use owned types like `String`, `Vec`, etc.?" Unless owned types are required, it's more flexible to use `&str` instead of `String`, `&[T]` instead of `Vec<T>`, etc. That means you can use both the owned variant and the borrowed one at call sites, making the function overall friendlier yet again. d. "Can this function fail? If yes, would it benefit from concrete error types?" Since this is something that can fail in *many* numerous and different ways, it's not necessary here - but it's something to keep in mind for library code e. "Are my defaults semver-safe [3]?" This is something that's often overlooked, but changing default parameters actually does break semver. E.g. it makes a huge difference if `compress` was set to `false`, but now it's `true` by default. This can affect callers in many unexpected ways, so make sure to choose your defaults very carefully. (It's also one of the reasons why Rust doesn't allow default values in functions, AFAIK.) Relevant if you go down the arg-struct route. There are probably many more things to think of, but these things immediately came to my mind, as they're IMO the most important things to consider. > +pub fn backup_vma_to_pbs( > + vma_file_path: Option<String>, Should be a `std::path::Path` or `PathBuf`. > + pbs_repository: String, > + backup_id: String, > + pbs_password: String, > + keyfile: Option<String>, > + key_password: Option<String>, > + master_keyfile: Option<String>, > + fingerprint: String, > + compress: bool, > + encrypt: bool, > +) -> Result<()> { > + println!("PBS repository: {}", pbs_repository); > + println!("PBS fingerprint: {}", fingerprint); > + println!("compress: {}", compress); > + println!("encrypt: {}", encrypt); > + The code from here ... > + let backup_time = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); > + println!("backup time: {}", backup_time); > + > + let mut pbs_err: *mut c_char = ptr::null_mut(); > + > + let pbs_repository_cstr = CString::new(pbs_repository)?; > + let backup_id_cstr = CString::new(backup_id)?; > + let pbs_password_cstr = CString::new(pbs_password)?; > + let fingerprint_cstr = CString::new(fingerprint)?; > + let keyfile_cstr = keyfile.map(|v| CString::new(v).unwrap()); > + let keyfile_ptr = keyfile_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > + let key_password_cstr = key_password.map(|v| CString::new(v).unwrap()); > + let key_password_ptr = key_password_cstr.map(|v| v.as_ptr()).unwrap_or(ptr::null()); > + let master_keyfile_cstr = master_keyfile.map(|v| CString::new(v).unwrap()); > + let master_keyfile_ptr = master_keyfile_cstr > + .map(|v| v.as_ptr()) > + .unwrap_or(ptr::null()); > + > + let pbs = proxmox_backup_new_ns( > + pbs_repository_cstr.as_ptr(), > + ptr::null(), > + backup_id_cstr.as_ptr(), > + backup_time, > + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE, > + pbs_password_cstr.as_ptr(), > + keyfile_ptr, > + key_password_ptr, > + master_keyfile_ptr, > + true, > + false, > + fingerprint_cstr.as_ptr(), > + &mut pbs_err, > + ); ... to here could e.g. be made its own function - if you had an arg-struct here, it would be easier to pass around all the original arguments between such helpers. Furthermore, if you make that function return a `Result<..., anyhow::Error>` (just use `anyhow::Result`) you can then add `Context` more conveniently, e.g.: let pbs = pbs_new_backup_namespaced(&cli_args, backup_time, ps_err) .context("failed to create new backups"); .. or something similar. You could also avoid the `unwrap()`s that way and just `?` on failure when converting to a `CString`. > + > + defer! { > + proxmox_backup_disconnect(pbs); > + } > + > + if pbs.is_null() { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}")); Use `anyhow::bail` [4] instead here. > + } The `unsafe` block above should only contain the part that's actually `unsafe`, and since you're doing the same kind of error handling in multiple locations, you should put it into a separate *inner* helper function with a comment regarding safety --> Why is it safe to make a `CStr` from the `pbs_err` pointer? Is the pointer guaranteed to *not* be `NULL` when an error happens? You're using `ptr::null_mut()` above after all. Otherwise the helper function should maybe also contain a null check. > + } > + > + let connect_result = proxmox_backup_connect(pbs, &mut pbs_err); > + > + if connect_result < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!("proxmox_backup_connect failed: {pbs_err_cstr:?}")); > + } > + } See above. > + > + println!("Connected to Proxmox Backup Server"); > + > + let vma_file: Box<dyn BufRead> = match vma_file_path { > + Some(vma_file_path) => match File::open(vma_file_path) { > + Err(why) => return Err(anyhow!("Couldn't open file: {}", why)), Use `anyhow::bail`. [4] > + Ok(file) => Box::new(BufReader::new(file)), > + }, > + None => Box::new(BufReader::new(stdin())), > + }; Maybe `if let Some(...)` would be better here? Also, you could try moving the `Box::new(BufReader::new())` outside of the expression, if it makes the code more concise. > + let mut vma_reader = VmaReader::new(vma_file)?; Would it make sense to add `Context` here? > + > + let start_transfer_time = SystemTime::now(); > + > + // Handle configs What does "handling configs" entail? Should probably be a separate function plus docstring. > + let configs = vma_reader.get_configs(); > + for (config_name, config_data) in configs { > + println!("CFG: size: {} name: {}", config_data.len(), config_name); > + > + let config_name_cstr = CString::new(config_name)?; > + > + if proxmox_backup_add_config( > + pbs, > + config_name_cstr.as_ptr(), > + config_data.as_ptr(), > + config_data.len() as u64, > + &mut pbs_err, > + ) < 0 As you did above, you could assign the result of `proxmox_backup_add_config` to a variable first to make the if-clause a little less thick. > + { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_add_config failed: {pbs_err_cstr:?}" > + )); > + } Use `anyhow::bail` [4] + helper to get the error string, as mentioned above. > + } > + } > + > + let mut pbs_device_id_map = [-1i32; 256]; > + let mut device_sizes = [0u64; 256]; Type aliases + constants for the sizes of these arrays here would be nice, e.g.: type DeviceSize = u64; const DEVICE_COUNT: usize = 256; > + > + // Handle block devices > + for device_id in 0..255 { Another place where a constant could be used - is it one you maybe have defined previously already somewhere? > + let device_name = match vma_reader.get_device_name(device_id) { > + Some(x) => x, > + None => continue, > + }; > + > + let device_size = match vma_reader.get_device_size(device_id) { > + Some(x) => x, > + None => continue, > + }; > + > + device_sizes[device_id as usize] = device_size; > + > + println!( > + "DEV: dev_id={} size: {} devname: {}", > + device_id, device_size, device_name > + ); > + > + let device_name_cstr = CString::new(device_name)?; > + let pbs_device_id = proxmox_backup_register_image( > + pbs, > + device_name_cstr.as_ptr(), > + device_size, > + false, > + &mut pbs_err, > + ); > + > + if pbs_device_id < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_register_image failed: {pbs_err_cstr:?}" > + )); > + } Use `anyhow::bail` [4] + helper function, as mentioned above. > + } > + > + pbs_device_id_map[device_id as usize] = pbs_device_id; > + } Does it make sense to put this into a separate function as well? > + > + struct ImageChunk { > + sub_chunks: HashMap<u8, Option<Vec<u8>>>, > + mask: u64, > + non_zero_mask: u64, > + } > + > + let images_chunks: RefCell<HashMap<u8, HashMap<u64, ImageChunk>>> = > + RefCell::new(HashMap::new()); Would it be simpler to factor this nested `HashMap` into its own type that describes what it's actually representing? That would allow you to express what's going on later in this function much better. > + > + vma_reader.restore(|dev_id, offset, buffer| { > + let pbs_device_id = pbs_device_id_map[dev_id as usize]; > + > + if pbs_device_id < 0 { > + return Err(anyhow!( > + "Reference to unknown device id {} in VMA file", > + dev_id > + )); Use `anyhow::bail` [4]. > + } > + > + let mut images_chunks = images_chunks.borrow_mut(); > + let image_chunks = images_chunks.get_mut(&dev_id); > + let pbs_chunk_offset = > + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE * (offset / PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE); > + let sub_chunk_index = > + ((offset % PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE) / VMA_CLUSTER_SIZE as u64) as u8; > + > + let pbs_chunk_size = > + PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE.min(device_sizes[dev_id as usize] - pbs_chunk_offset); While I trust that you calculations are fine, it would be nice if the above was more descriptive --> Separate function with docstring, perhaps? > + > + let pbs_upload_chunk = |pbs_chunk_buffer: Option<&[u8]>| { Can this closure be a function? > + println!( > + "Uploading dev_id: {} offset: {:#0X} - {:#0X}", > + dev_id, > + pbs_chunk_offset, > + pbs_chunk_offset + pbs_chunk_size, > + ); > + > + let mut pbs_err: *mut c_char = ptr::null_mut(); > + > + let write_data_result = proxmox_backup_write_data( > + pbs, > + pbs_device_id as u8, > + match pbs_chunk_buffer { > + Some(pbs_chunk_buffer) => pbs_chunk_buffer.as_ptr(), > + None => ptr::null(), > + }, > + pbs_chunk_offset, > + pbs_chunk_size, > + &mut pbs_err, > + ); > + > + if write_data_result < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_write_data failed: {pbs_err_cstr:?}" > + )); > + } Use `anyhow::bail` [4] + helper for error message, as above. > + } > + > + Ok(()) > + }; > + > + let insert_image_chunk = |image_chunks: &mut HashMap<u64, ImageChunk>, > + buffer: Option<Vec<u8>>| { Should probably also be a function instead of a closure? > + let mut sub_chunks: HashMap<u8, Option<Vec<u8>>> = HashMap::new(); > + let mask = 1 << sub_chunk_index; > + let non_zero_mask = buffer.is_some() as u64; > + sub_chunks.insert(sub_chunk_index, buffer); > + > + let image_chunk = ImageChunk { > + sub_chunks, > + mask, > + non_zero_mask, > + }; > + > + image_chunks.insert(pbs_chunk_offset, image_chunk); > + }; > + > + match image_chunks { > + Some(image_chunks) => { > + let image_chunk = image_chunks.get_mut(&pbs_chunk_offset); > + > + match image_chunk { > + Some(image_chunk) => { > + image_chunk.mask |= 1 << sub_chunk_index; > + image_chunk.non_zero_mask |= (buffer.is_some() as u64) << sub_chunk_index; > + image_chunk.sub_chunks.insert(sub_chunk_index, buffer); > + > + let sub_chunk_count = > + ((pbs_chunk_size + 65535) / VMA_CLUSTER_SIZE as u64) as u8; Didn't you declare a constant for that value above? > + let pbs_chunk_mask = 1_u64 > + .checked_shl(sub_chunk_count.into()) > + .unwrap_or(0) > + .wrapping_sub(1); > + > + if image_chunk.mask == pbs_chunk_mask { > + if image_chunk.non_zero_mask == 0 { > + pbs_upload_chunk(None)?; > + } else { > + let mut pbs_chunk_buffer = > + proxmox_io::boxed::zeroed(pbs_chunk_size as usize); > + > + for i in 0..sub_chunk_count { > + let sub_chunk = image_chunk.sub_chunks.get(&i).unwrap(); > + let a = i as usize * VMA_CLUSTER_SIZE; What is "a"? > + let b = (a + VMA_CLUSTER_SIZE).min(pbs_chunk_size as usize); What is "b"? > + > + match sub_chunk { > + Some(sub_chunk) => { > + pbs_chunk_buffer[a..b] > + .copy_from_slice(&sub_chunk[0..b - a]); > + } > + None => pbs_chunk_buffer[a..b].fill(0), > + } > + } > + > + pbs_upload_chunk(Some(&*pbs_chunk_buffer))?; > + } > + > + image_chunks.remove(&pbs_chunk_offset); > + } > + } > + None => { > + if pbs_chunk_size <= VMA_CLUSTER_SIZE as u64 { > + pbs_upload_chunk(buffer.as_deref())?; `anyhow::Context` here would be nice. Why can this fail? > + } else { > + insert_image_chunk(image_chunks, buffer); > + } > + } > + } > + } > + None => { > + if pbs_chunk_size <= VMA_CLUSTER_SIZE as u64 { > + pbs_upload_chunk(buffer.as_deref())?; `anyhow::Context` here, too. > + } else { > + let mut image_chunks: HashMap<u64, ImageChunk> = HashMap::new(); > + insert_image_chunk(&mut image_chunks, buffer); > + images_chunks.insert(dev_id, image_chunks); > + } > + } > + } > + > + Ok(()) > + })?; This *entire* closure deserves to be broken up into multiple smaller parts. It's really hard to follow the code at all, IMHO. You might even get rid of the `RefCell`s that way, perhaps. > + > + for pbs_device_id in pbs_device_id_map.iter().take(255) { Magic constant again here too. > + if *pbs_device_id < 0 { > + continue; > + } > + > + if proxmox_backup_close_image(pbs, *pbs_device_id as u8, &mut pbs_err) < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!( > + "proxmox_backup_close_image failed: {pbs_err_cstr:?}" > + )); > + } Use `anyhow::bail` [4] + helper for error message, as above. > + } > + } > + > + if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { > + unsafe { > + let pbs_err_cstr = CStr::from_ptr(pbs_err); > + return Err(anyhow!("proxmox_backup_finish failed: {pbs_err_cstr:?}")); > + } Use `anyhow::bail` [4] + helper for error message, as above. > + } > + > + println!( > + "Backup finished within {}ms", > + SystemTime::now() > + .duration_since(start_transfer_time)? > + .as_millis() > + ); Assign the result of `SystemTime::now()...` to a variable, so you can just: println!("Backup finished in {ms} ms."); Also, why milliseconds and not minutes + seconds? > + > + Ok(()) > +} [0]: https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html [1]: https://doc.rust-lang.org/std/io/prelude/index.html [2]: https://doc.rust-lang.org/std/prelude/index.html [3]: https://semver.org/ [4]: https://docs.rs/anyhow/1.0.80/anyhow/macro.bail.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin 2024-03-06 15:49 ` Max Carrara @ 2024-03-12 14:04 ` Filip Schauer 0 siblings, 0 replies; 14+ messages in thread From: Filip Schauer @ 2024-03-12 14:04 UTC (permalink / raw) To: Max Carrara, Proxmox Backup Server development discussion On 06/03/2024 16:49, Max Carrara wrote: > Regarding `F: Fn(u8, u64, Option<Vec<u8>>) -> Result<()>`: > * Why `Fn` and not `FnOnce`? You call this with a closure later on. It is called multiple times in the for loop in restore_extent. >> + let is_zero = blockinfo.mask == 0; > I'm usually in favour of assigning the result of conditional checks > to variables first, but is that really necessary here? > >> >> - while file_offset < vma_file_size { >> - self.vma_file.seek(SeekFrom::Start(file_offset))?; >> - let vma_extent_header = Self::read_extent_header(&mut self.vma_file)?; >> - file_offset += size_of::<VmaExtentHeader>() as u64; >> + let image_chunk_buffer = if is_zero { > It's only used here after all, and inlining it wouldn't make the code > more complex at all. Not necessary but it improves readability in my opinion. > Also, > `Vec<u8>` could probably be `&[u8]`, couldn't it? No, because then the image_chunk_buffer would not live long enough. The chunk needs to live past func so it can be stored in the images_chunks HashMap in vma2pbs.rs. This HashMap can hold multiple chunks before they are sent off as one big chunk to the PBS. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH v4 vma-to-pbs 6/6] Add a fallback for the --fingerprint argument 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer ` (4 preceding siblings ...) 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin Filip Schauer @ 2024-03-05 13:56 ` Filip Schauer 2024-03-06 15:49 ` [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Max Carrara 2024-03-20 14:18 ` Filip Schauer 7 siblings, 0 replies; 14+ messages in thread From: Filip Schauer @ 2024-03-05 13:56 UTC (permalink / raw) To: pbs-devel Fallback to the PBS_FINGERPRINT environment variable if the --fingerprint argument is not specified. Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- Cargo.toml | 2 +- src/main.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9711690..f56e351 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ edition = "2021" [dependencies] anyhow = "1.0" bincode = "1.3" -clap = { version = "4.0.32", features = ["cargo"] } +clap = { version = "4.0.32", features = ["cargo", "env"] } md5 = "0.7.0" scopeguard = "1.1.0" serde = "1.0" diff --git a/src/main.rs b/src/main.rs index dc8171e..c458c4d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -31,7 +31,7 @@ fn main() -> Result<()> { .long("fingerprint") .value_name("FINGERPRINT") .help("Proxmox Backup Server Fingerprint") - .required(true), + .env("PBS_FINGERPRINT"), ) .arg( Arg::new("keyfile") @@ -76,9 +76,10 @@ fn main() -> Result<()> { let pbs_repository = matches.get_one::<String>("repository").unwrap().to_string(); let vmid = matches.get_one::<String>("vmid").unwrap().to_string(); + let fingerprint = matches .get_one::<String>("fingerprint") - .unwrap() + .expect("Fingerprint not set. Use $PBS_FINGERPRINT or --fingerprint") .to_string(); let keyfile = matches.get_one::<String>("keyfile"); -- 2.39.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer ` (5 preceding siblings ...) 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 6/6] Add a fallback for the --fingerprint argument Filip Schauer @ 2024-03-06 15:49 ` Max Carrara 2024-03-06 15:57 ` Wolfgang Bumiller 2024-03-20 14:18 ` Filip Schauer 7 siblings, 1 reply; 14+ messages in thread From: Max Carrara @ 2024-03-06 15:49 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Filip Schauer, Wolfgang Bumiller On 3/5/24 14:56, Filip Schauer wrote: > Implement a tool to import VMA files into a Proxmox Backup Server First of all, I get that patches 1-3 have been applied already, but I still wanted to point out some things in patch 1. How did you build and test this series? proxmox-schema is now at version 3, but proxmox-backup-qemu still requires version 2. Manually installing version 2 breaks some other dependencies - resolving them by hand was honestly too tedious at the moment. If there's any help needed to bump the version, let me know; I might be able to lend a hand there (@Wolfgang). Regarding the code itself, I really like the overall approach a lot, but IMHO the code needs a style refresher. Some things would benefit from being broken up into smaller parts to reduce the overall complexity for a given function - makes things easier to parse and follow (and also easier to debug in case something goes wrong). Additionally, error handling gets *much* easier that way; more below. There are also some bits that could be de-duplicated and some things regarding generics and dynamic dispatch - it's best if you read the comments inline. Regarding error handling: A recurring theme is that `anyhow` isn't utilized fully - it's really great that you got rid of a lot of `unwrap()`s later in the series, but "idiomatic" `anyhow` is used best with `anyhow::Context` [0] and generous use of the `?` operator. Smaller functions have the benefit that they define an "error handling boundary" of some sort, if they return `Result<T, anyhow::Error>`. It's often better to then add `Context` to those helper functions - that way you can also avoid adding `Context` to *every* single little instance where you might be doing something in particular. See comments inline, in particular patch 5, for a better explanation. Also, sorry if this all seems a little excessive! I wanted to be as thorough as possible. [0]: https://docs.rs/anyhow/latest/anyhow/trait.Context.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool 2024-03-06 15:49 ` [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Max Carrara @ 2024-03-06 15:57 ` Wolfgang Bumiller 0 siblings, 0 replies; 14+ messages in thread From: Wolfgang Bumiller @ 2024-03-06 15:57 UTC (permalink / raw) To: Max Carrara; +Cc: Proxmox Backup Server development discussion, Filip Schauer On Wed, Mar 06, 2024 at 04:49:45PM +0100, Max Carrara wrote: > On 3/5/24 14:56, Filip Schauer wrote: > > Implement a tool to import VMA files into a Proxmox Backup Server > > First of all, I get that patches 1-3 have been applied already, > but I still wanted to point out some things in patch 1. > > > How did you build and test this series? proxmox-schema is now at > version 3, but proxmox-backup-qemu still requires version 2. > Manually installing version 2 breaks some other dependencies - > resolving them by hand was honestly too tedious at the moment. > > If there's any help needed to bump the version, let me know; I might > be able to lend a hand there (@Wolfgang). sorry about that - just pushed an update to proxmox-backup-qemu repo, submodule in vma-to-pbs just needs to be bumped now ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer ` (6 preceding siblings ...) 2024-03-06 15:49 ` [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Max Carrara @ 2024-03-20 14:18 ` Filip Schauer 7 siblings, 0 replies; 14+ messages in thread From: Filip Schauer @ 2024-03-20 14:18 UTC (permalink / raw) To: pbs-devel Patch v5 is available: https://lists.proxmox.com/pipermail/pbs-devel/2024-March/008178.html On 05/03/2024 14:56, Filip Schauer wrote: > Implement a tool to import VMA files into a Proxmox Backup Server > > Example usage: > > zstd -d --stdout vzdump.vma.zstd | vma-to-pbs \ > --repository <auth_id@host:port:datastore> \ > --vmid 123 \ > --password_file pbs_password > > Changes since v3: > * Add the ability to provide credentials via files > * Add support for streaming the VMA file via stdin > * Add a fallback for the --fingerprint argument > > Changes since v2: > * Use the deb packages from the proxmox-io and proxmox-sys dependencies > instead of the proxmox submodule > * Remove the proxmox submodule > * Update the proxmox-backup-qemu submodule to make it buildable with > the newest librust dependencies > > Changes since v1: > * Remove unused crates and uses > * Format the code > * Use anyhow for error handling > * Use clap for parsing arguments instead of getopts > * Fix blocks being reindexed on every read > * Make sure ProxmoxBackupHandle is dropped properly on error > * Move image_chunk_buffer from stack to heap > * Move the block_index in VmaReader to the heap completely > * Initialize vectors with `Vec::with_capacity` and `resize` instead of > the `vec!` macro, to potentially improve performance on debug builds. > * Add comments to code filling the MD5 sum field with zeros > * Change device_id arguments to usize > * Handle devices that have a size that is not aligned to 4096 properly > in read_device_contents, when the caller provides a buffer that would > exceed the device size. > * Avoid unnecessary loop iterations in read_device_contents when the > buffer size is not aligned to 65536 > > Filip Schauer (4): > Initial commit > Add the ability to provide credentials via files > Add support for streaming the VMA file via stdin > Add a fallback for the --fingerprint argument > > Wolfgang Bumiller (2): > cargo fmt > bump proxmox-backup-qemu > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-03-20 14:18 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-05 13:56 [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit Filip Schauer 2024-03-06 15:49 ` Max Carrara 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 2/6] cargo fmt Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 3/6] bump proxmox-backup-qemu Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 4/6] Add the ability to provide credentials via files Filip Schauer 2024-03-06 15:49 ` Max Carrara 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 5/6] Add support for streaming the VMA file via stdin Filip Schauer 2024-03-06 15:49 ` Max Carrara 2024-03-12 14:04 ` Filip Schauer 2024-03-05 13:56 ` [pbs-devel] [PATCH v4 vma-to-pbs 6/6] Add a fallback for the --fingerprint argument Filip Schauer 2024-03-06 15:49 ` [pbs-devel] [PATCH v4 vma-to-pbs 0/6] Implement vma-to-pbs tool Max Carrara 2024-03-06 15:57 ` Wolfgang Bumiller 2024-03-20 14:18 ` Filip Schauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox