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

[Tomáš Golembiovský]

On Wed, 7 Nov 2018 15:43:00 +0000
"Richard W.M. Jones" <rjones at redhat.com> wrote:

> 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.

done

> 
> > +(* 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.

I don't believe it matters.

The comments below were applied in separate commit.

    Tomas

> 
> >          )
> >        ) 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/


-- 
Tomáš Golembiovský <tgolembi at redhat.com>