From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <m.carrara@proxmox.com>
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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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: <D0AKM2V2R5J9.1UVDHKAI2JSTD@proxmox.com>
To: "Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>
From: "Max Carrara" <m.carrara@proxmox.com>
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
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=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 <auth_id@host:port:datastore> \
> --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<T, Error>` 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