Libguestfs - Dec 2021 - [v2v PATCH] output_rhv: restrict block status collection to the old RHV output

On Mon, Dec 20, 2021 at 02:56:40PM +0100, Laszlo Ersek
wrote:
> +        if (need_actual_sizes) then
You don't need (and shouldn't use) the parentheses here.
> +          (* Ovirt-engine considers the "ovf:actual_size"
attribute mandatory.
> +           * If we don't know the actual size, we must create the
attribute
> +           * with empty contents.
> +           *)
> +          List.push_back attrs
> +            ("ovf:actual_size",
> +             match actual_size with
> +              | None -> ""
> +              | Some actual_size -> Int64.to_string (bytes_to_gb
actual_size)
FYI I was going to suggest (untested!):

  actual_size |>
    Option.map (bytes_to_gb |> Int64.to_string) |>
    Option.default ""

but actually once I wrote it out like this it's pretty clunky and
obscure.  If the match was less complex it could make sense to
consider this, but I don't think in this case.

Rest of the patch is all good, thanks, so:

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 12/21/21 14:47, Richard W.M. Jones wrote:
> On Mon, Dec 20, 2021 at 02:56:40PM +0100, Laszlo Ersek wrote:
>> +        if (need_actual_sizes) then
>
> You don't need (and shouldn't use) the parentheses here.
Ah, you are totally right -- "muscle memory" from C. I've been
getting
better at not using parens with "if"s in OCaml, but I still fail to
catch myself sometimes. Funnily enough, in this single patch, I wrote
*both* "if need_actual_sizes then" *and* "if (need_actual_sizes)
then".
>
>> +          (* Ovirt-engine considers the "ovf:actual_size"
attribute mandatory.
>> +           * If we don't know the actual size, we must create the
attribute
>> +           * with empty contents.
>> +           *)
>> +          List.push_back attrs
>> +            ("ovf:actual_size",
>> +             match actual_size with
>> +              | None -> ""
>> +              | Some actual_size -> Int64.to_string (bytes_to_gb
actual_size)
>
> FYI I was going to suggest (untested!):
>
>   actual_size |>
>     Option.map (bytes_to_gb |> Int64.to_string) |>
>     Option.default ""
Interesting, thanks for teaching me this!

... after consulting <https://ocaml.org/api/Option.html>, is the last
function "Option.value", rather than "Option.default"?
>
> but actually once I wrote it out like this it's pretty clunky and
> obscure.  If the match was less complex it could make sense to
> consider this, but I don't think in this case.
>
> Rest of the patch is all good, thanks, so:
>
> Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
I've removed the superfluous parens, and have re-run "make check".

$ git range-diff --color \
    master \
    restrict-actual-size-to-rhv-upload-rhbz-2034240-v1 \
    apply
> 1:  9695e6f2394f ! 1:  07b12fe99fb9 output_rhv: restrict block status
collection to the old RHV output
>     @@ -23,6 +23,10 @@
>          Suggested-by: Richard W.M. Jones <rjones at redhat.com>
>          Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240
>          Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>     +    Message-Id: <20211220135640.12436-1-lersek at redhat.com>
>     +    Tested-by: Nir Soffer <nsoffer at redhat.com>
>     +    [lersek at redhat.com: drop parens around condition in
"if"; thanks: Rich]
>     +    Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
>
>      diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli
>      --- a/lib/create_ovf.mli
>     @@ -97,7 +101,7 @@
>      -            | None -> ""
>      -            | Some actual_size -> Int64.to_string (bytes_to_gb
actual_size)
>      -          );
>     -+        if (need_actual_sizes) then
>     ++        if need_actual_sizes then
>      +          (* Ovirt-engine considers the "ovf:actual_size"
attribute mandatory.
>      +           * If we don't know the actual size, we must create the
attribute
>      +           * with empty contents.
Commit 07b12fe99fb9.

Thanks!
Laszlo