[pbs-devel] [PATCH v4 vma-to-pbs 1/6] Initial commit
Max Carrara
m.carrara at proxmox.com
Wed Mar 6 16:49:19 CET 2024
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 at 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 at 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 at 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
More information about the pbs-devel
mailing list