[Libguestfs] [PATCH v2v 1/3] qemu-nbd: Implement output compression for qcow2 files

[Richard W.M. Jones]

On Tue, Jul 05, 2022 at 08:51:49AM +0200, Laszlo Ersek wrote:
> On 07/01/22 13:01, Richard W.M. Jones wrote:
> > ---
> >  lib/qemuNBD.ml    | 11 +++++++++--
> >  lib/qemuNBD.mli   |  5 +++++
> >  output/output.ml  | 38 +++++++++++++++++++++++++++++++++++---
> >  output/output.mli |  1 +
> >  4 files changed, 50 insertions(+), 5 deletions(-)
> 
> (Can you update your git diff order so that *.mli be put before *.ml?)

In nbdkit we have scripts/git.orderfile:

https://gitlab.com/nbdkit/nbdkit/-/blob/master/scripts/git.orderfile

I guess we should copy this into other projects.

...
> > +         let () =
> 
> What is the purpose of "let ()" here specifically?
> 
> (I know what it's good for in general, from your earlier reference
> <https://baturin.org/docs/ocaml-faq/>, but I don't remember seeing it
> much outside of the outermost scope.)
>
> > +           let version = NBD.create () |> NBD.get_version in
> > +           let version = String.nsplit "." version in
> > +           let version = List.map int_of_string version in
> > +           if version < [1; 13; 5] then

It hides the binding of "version", so it doesn't escape from this
scope.  It's not necessary, but avoids possible confusion if we used
an unrelated "version" symbol somewhere later in the code.

> (Huh, didn't know such comparison existed, great!)

Sure, you can compare anything except recursive structures, functions
(because of the Halting Problem IIRC) and certain peculiar internal
types.  It's nothing clever, it just recursively compares the two
structures until it finds something unequal:

https://github.com/ocaml/ocaml/blob/d9afa408c612e74a266b95f0fa25bb1efde72112/runtime/compare.c#L110

> Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks - I'll address the other comments you made in the updated patch.

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