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

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(a)redhat.com&gt; > --- > 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/