Re: [Libguestfs] [v2v PATCH] output_rhv: restrict block status collection to the old RHV output
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(a)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(a)redhat.com>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
+ Message-Id: <20211220135640.12436-1-lersek(a)redhat.com>
+ Tested-by: Nir Soffer <nsoffer(a)redhat.com>
+ [lersek(a)redhat.com: drop parens around condition in "if"; thanks:
Rich]
+ Reviewed-by: Richard W.M. Jones <rjones(a)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