[Libguestfs] [PATCH v3 1/3] v2v: refactor copy_drivers() in Windows_virtio

[Richard W.M. Jones]

On Wed, Nov 07, 2018 at 12:53:18PM +0100, Tomáš Golembiovský wrote:
> Changed the function to be more generic and renamed.
> The only change in behavior is in produced debug messages.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi at redhat.com>
> ---
>  v2v/windows_virtio.ml | 48 ++++++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 9b45c76f5..da02b6c4e 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -254,28 +254,41 @@ and ddb_regedits inspect drv_name drv_pciid =
>   * been copied.
>   *)
>  and copy_drivers g inspect driverdir =
> -  let ret = ref false in
> +  List.length (
> +    copy_from_virtio_win g inspect "/" driverdir virtio_iso_path_matches_guest_os
> +  ) > 0

You can write this as:

  [] <> copy_from_virtio_win g [etc...]

which is also more efficient because the compiler will optimize it to
a single test that the function doesn't return a cons.

> +(* Copy all files from virtio_win directory/ISO located in [srcdir]
> + * subdirectory and all its subdirectories to the [destdir]. The directory
> + * hierarchy is not preserved, meaning all files will be directly in [destdir].
> + * The file list is filtered based on [filter] function.
> + *
> + * Returns list of copied files.
> + *)
> +and copy_from_virtio_win g inspect srcdir destdir filter =
> +  let ret = ref [] in
>    if is_directory virtio_win then (
> -    debug "windows: copy_drivers: source directory virtio_win %s" virtio_win;
> +    let dir = virtio_win // srcdir in
> +    debug "windows: copy_from_virtio_win: guest tools source directory %s" dir;
>  
> -    let cmd = sprintf "cd %s && find -L -type f" (quote virtio_win) in
> +    let cmd = sprintf "cd %s && find -L -type f" (quote dir) in
>      let paths = external_command cmd in
>      List.iter (
>        fun path ->
> -        if virtio_iso_path_matches_guest_os path inspect then (
> -          let source = virtio_win // path in
> -          let target = driverdir //
> -                         String.lowercase_ascii (Filename.basename path) in
> -          debug "copying virtio driver bits: 'host:%s' -> '%s'"
> +        if filter path inspect then (
> +          let source = dir // path in
> +          let target_name = String.lowercase_ascii (Filename.basename path) in
> +          let target = destdir // target_name in
> +          debug "windows: copying guest tools bits: 'host:%s' -> '%s'"
>                  source target;
>  
>            g#write target (read_whole_file source);
> -          ret := true
> +          List.push_front target_name ret

This will return the list of files backwards (if that matters), but is
also more efficient than using push_back.

>          )
>        ) paths
>    )
>    else if is_regular_file virtio_win then (
> -    debug "windows: copy_drivers: source ISO virtio_win %s" virtio_win;
> +    debug "windows: copy_from_virtio_win: guest tools source ISO %s" virtio_win;
>  
>      try
>        let g2 = open_guestfs ~identifier:"virtio_win" () in
> @@ -283,19 +296,20 @@ and copy_drivers g inspect driverdir =
>        g2#launch ();
>        let vio_root = "/" in
>        g2#mount_ro "/dev/sda" vio_root;
> -      let paths = g2#find vio_root in
> +      let srcdir = vio_root // srcdir in

It doesn't really matter since virt-v2v only ever runs on Linux,
but strictly speaking this is wrong.  It should be:

  let srcdir = vio_root ^ "/" ^ srcdir in

(But using // in the previous part of the code *was* correct).  The
reason is that this path is evaluated in the libguestfs appliance
name space, whereas operation // uses host path concatenation
(would be \ on Windows host for example).

> +      let paths = g2#find srcdir in
>        Array.iter (
>          fun path ->
> -          let source = vio_root // path in
> +          let source = srcdir // path in

Same here, except the old code was wrong too :-)

>            if g2#is_file source ~followsymlinks:false &&
> -               virtio_iso_path_matches_guest_os path inspect then (
> -            let target = driverdir //
> -                           String.lowercase_ascii (Filename.basename path) in
> -            debug "copying virtio driver bits: '%s:%s' -> '%s'"
> +               filter path inspect then (
> +            let target_name = String.lowercase_ascii (Filename.basename path) in
> +            let target = destdir // target_name in

And this is wrong too (and in the old code).

> +            debug "windows: copying guest tools bits: '%s:%s' -> '%s'"
>                    virtio_win path target;
>  
>              g#write target (g2#read_file source);
> -            ret := true
> +            List.push_front target_name ret
>            )
>          ) paths;
>        g2#close()
> -- 

Generally looks fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/