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

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

On 3/5/24 14:56, Filip Schauer wrote:
> Implement a tool to import VMA files into a Proxmox Backup Server

First of all, I get that patches 1-3 have been applied already,
but I still wanted to point out some things in patch 1.


How did you build and test this series? proxmox-schema is now at
version 3, but proxmox-backup-qemu still requires version 2.
Manually installing version 2 breaks some other dependencies -
resolving them by hand was honestly too tedious at the moment.

If there's any help needed to bump the version, let me know; I might
be able to lend a hand there (@Wolfgang).


Regarding the code itself, I really like the overall approach a lot,
but IMHO the code needs a style refresher. Some things would benefit
from being broken up into smaller parts to reduce the overall
complexity for a given function - makes things easier to parse and
follow (and also easier to debug in case something goes wrong).
Additionally, error handling gets *much* easier that way; more below.

There are also some bits that could be de-duplicated and some things
regarding generics and dynamic dispatch - it's best if you read the
comments inline.


Regarding error handling: A recurring theme is that `anyhow` isn't utilized
fully - it's really great that you got rid of a lot of `unwrap()`s later in
the series, but "idiomatic" `anyhow` is used best with `anyhow::Context` [0]
and generous use of the `?` operator.

Smaller functions have the benefit that they define an "error handling boundary"
of some sort, if they return `Result<T, anyhow::Error>`.

It's often better to then add `Context` to those helper functions - that
way you can also avoid adding `Context` to *every* single little instance
where you might be doing something in particular. See comments inline, in
particular patch 5, for a better explanation.


Also, sorry if this all seems a little excessive! I wanted to be as thorough
as possible.


[0]: https://docs.rs/anyhow/latest/anyhow/trait.Context.html