Re: [pbs-devel] [PATCH vma-to-pbs 0/9] Implement vma-to-pbs tool

["Max Carrara" <m.carrara@proxmox.com>]

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 readability
>
> 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