[Libguestfs] [PATCH v2 2/4] v2v: copy virtio drivers without guestfs handle leak

[Richard W.M. Jones]

On Tue, Oct 13, 2015 at 03:50:52PM +0300, Roman Kagan wrote:
> Refactor copying of virtio windows drivers into the guest so that the
> matching of the drivers to the guest os flavor and copying the files
> happens one next to the other in a single function, and no guestfs
> handle (nor any other irrelevant info) is leaked outside.
> 
> Signed-off-by: Roman Kagan <rkagan at virtuozzo.com>

This one makes sense too, apart from a few style-only problems:

> +    if is_directory virtio_win then (
> +      let cmd = sprintf "cd %s && find -type f" (quote virtio_win) in
> +      let paths = external_command cmd in
> +      List.iter (
> +        fun path ->
> +          if (match_vio_path_with_os path inspect.i_arch
> +              inspect.i_major_version inspect.i_minor_version
> +              inspect.i_product_variant) then (

Don't need parens around if conditions, and
it's easier to read this statement if the parameters
are indented:

          if match_vio_path_with_os path inspect.i_arch
                                    inspect.i_major_version
                                    inspect.i_minor_version
                                    inspect.i_product_variant then (

> +            if (g2#is_file source ~followsymlinks:false) &&
> +               (match_vio_path_with_os path inspect.i_arch
> +                inspect.i_major_version inspect.i_minor_version
> +                inspect.i_product_variant) then (

As above: you don't need parens around the parameters of &&,
and more indentation needed of match_vio_path_with_os.

> +              let target = driverdir // (String.lowercase_ascii
> +                                          (Filename.basename path)) in

Because function application binds most tightly, you can write this
as:

      let target = driverdir //
                     String.lowercase_ascii (Filename.basename path) in

> +(* Given a path of a file relative to the root of the directory tree with
> + * virtio-win drivers, figure out if it's suitable for the specific Windows
> + * flavor.
> + *)
> +let match_vio_path_with_os path arch os_major os_minor os_variant =

Be nice if this function had a better, shorter name, but I don't know
what that name should be :-/  I think the problem with this function is
the name starts with 'match', which makes me think of the match
keyword every time I see it being used.

> +    (* Skip files without specific extensions. *)
> +    let extensions = ["cat"; "inf"; "pdb"; "sys"] in
> +    if (not (List.mem extension extensions)) then raise Not_found;

Parens around if condition.

Rest looks fine to me.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org