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

[Filip Schauer <f.schauer@proxmox.com>]

Patch v7 is available:

https://lists.proxmox.com/pipermail/pbs-devel/2024-April/008439.html

On 03/04/2024 16:51, Max Carrara wrote:
> 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
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>