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@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@redhat.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek@redhat.com> + Message-Id: <20211220135640.12436-1-lersek@redhat.com> + Tested-by: Nir Soffer <nsoffer@redhat.com> + [lersek@redhat.com: drop parens around condition in "if"; thanks: Rich] + Reviewed-by: Richard W.M. Jones <rjones@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