From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id CC400B8003 for ; Wed, 6 Mar 2024 16:49:25 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A0B481B981 for ; Wed, 6 Mar 2024 16:49:25 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 6 Mar 2024 16:49:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5677648839 for ; Wed, 6 Mar 2024 16:49:23 +0100 (CET) Message-ID: <8118c8d7-13c5-4886-bd21-2837e5e74480@proxmox.com> Date: Wed, 6 Mar 2024 16:49:19 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Filip Schauer References: <20240305135645.96347-1-f.schauer@proxmox.com> <20240305135645.96347-2-f.schauer@proxmox.com> Content-Language: en-US From: Max Carrara In-Reply-To: <20240305135645.96347-2-f.schauer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.003 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Mar 2024 15:49:25 -0000 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 > --- 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 "] > +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, > + key_password: Option, > + master_keyfile: Option, > + 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::("repository").unwrap().to_string(); > + let vmid = matches.get_one::("vmid").unwrap().to_string(); > + let fingerprint = matches.get_one::("fingerprint").unwrap().to_string(); > + > + let keyfile = matches.get_one::("keyfile"); > + let master_keyfile = matches.get_one::("master_keyfile"); > + let compress = matches.get_flag("compress"); > + let encrypt = matches.get_flag("encrypt"); > + > + let vma_file_path = matches.get_one::("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, > + block_index: Vec>, > + blocks_are_indexed: bool, > +} > + > +impl VmaReader { > + pub fn new(vma_file_path: &str) -> Result { > + 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> = (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 { > + let mut buffer = Vec::with_capacity(size_of::()); > + buffer.resize(size_of::(), 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::()..])?; > + > + // 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 { > + 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> { > + 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 { > + return self.configs.clone(); > + } > + > + pub fn get_device_name(&mut self, device_id: usize) -> Option { > + 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 { > + 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 { > + let mut buffer = Vec::with_capacity(size_of::()); > + buffer.resize(size_of::(), 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::() 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 { > + 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