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 108C7913E3 for ; Wed, 3 Apr 2024 16:51:45 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E41CC1B23D for ; Wed, 3 Apr 2024 16:51:44 +0200 (CEST) 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, 3 Apr 2024 16:51:43 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 1FD3C44B4E for ; Wed, 3 Apr 2024 16:51:43 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Wed, 03 Apr 2024 16:51:41 +0200 Message-Id: To: "Proxmox Backup Server development discussion" From: "Max Carrara" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240403094913.107177-1-f.schauer@proxmox.com> In-Reply-To: <20240403094913.107177-1-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, vma2pbs.rs, vma.rs, main.rs] Subject: Re: [pbs-devel] [PATCH vma-to-pbs 0/9] Implement vma-to-pbs tool 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, 03 Apr 2024 14:51:45 -0000 On Wed Apr 3, 2024 at 11:49 AM CEST, Filip Schauer wrote: > Implement a tool to import VMA files into a Proxmox Backup Server > > Example usage: > > zstd -d --stdout vzdump.vma.zst | vma-to-pbs \ > --repository \ > --vmid 123 \ > --password-file pbs_password > > Commit 2/9 requires > https://lists.proxmox.com/pipermail/pve-devel/2024-March/062182.html > to be applied first. You've made some excellent progress since the last version, very nicely done! There are a few more comments inline, but it's almost there. Some general notes: * The overall code is now much more readable ever since you broke up the few remaining pieces. (Especially that one fat closure I mentioned in v5). Great job! * If you introduce a change in one commit and then alter that change in a later commit in the same series, it's usually best to just introduce the final change all at once. This doesn't apply everywhere of course - I've left an example inline to show what I mean exactly. * In general we don't `use anyhow::Result;` and instead always opt to using `Result` with `use anyhow::Error;` - but no hard feelings on my side. Though, it would be great to follow this convention from the start. (Side note: We should probably write all of our conventions down somewhere, but that's not important now.) I was able to shove a few `.vma` files to my PBS instance just as you described above, so the converting-and-backing-up works as intended. The last couple things that IMO need changing are rather minor - so if you sort those out, this gets a definitive pass from me. Great to see how far this series has come along, you should be proud of yourself :) > > Changes since v5: > * Switch argument parsing from clap to pico-args > * Change time printing format ISO 8601 timestamps > * use anyhow::bail, so it does not have to be written out everytime > * Force the usage of password files when passing the vma file to stdin > * Cut off a trailing new line when reading a password from a file > * Extract PBS error handling into a seperate function > * Reformat command line arguments to kebab-case > * Refactor VmaReader to be generic over Read trait > * Split up block device upload into smaller functions to improve readabil= ity > > Changes since v4: > * Bump proxmox-backup-qemu > * Remove unnecessary "extern crate" declarations > * Refactor error handling with anyhow > * vma.rs: Improve code readability by adding constants and using more > descriptive variable/type names. > * vma.rs: Move duplicate code into read_string function > * Print elapsed time in minutes, seconds and ms > * Refactor block device id and size retrieval logic > * vma: Document break statement when reaching end of file > * Use selected imports instead of glob imports > * Split up vma2pbs logic into seperate functions > * Makefile: remove reference to unused submodule > > 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 (9): > Add the ability to provide credentials via files > bump proxmox-backup-qemu > remove unnecessary "extern crate" declarations > add support for streaming the VMA file via stdin > add a fallback for the --fingerprint argument > refactor error handling > makefile: remove reference to unused submodule > switch argument handling from clap to pico-args > reformat command line arguments to kebab-case > > Cargo.toml | 2 +- > Makefile | 2 +- > src/main.rs | 421 +++++++++++---------------------- > src/vma.rs | 343 ++++++++++++--------------- > src/vma2pbs.rs | 386 ++++++++++++++++++++++++++++++ > submodules/proxmox-backup-qemu | 2 +- > 6 files changed, 686 insertions(+), 470 deletions(-) > create mode 100644 src/vma2pbs.rs