[PATCH 0/4] v2v: simplify driver copying from virtio-win iso

[PATCH] Chop final '/' in output...

[PATCH 1/2] lib: info: Move common...

Roman Kagan

Monday, 10 August 2015 Mon, 10 Aug '15

10:55 a.m.

Libguestfs supports passing an ISO image as a source of virtio windows drivers to v2v. That support, however, looks too heavy-weight: in order to access those drivers, a separate guestfs handle is created (and thus a new emulator process is started), which runs until v2v completes. This series attempts to make it simpler and lighter-weight, by making the relevant code more local, and by hot-adding the image into the main guestfs handle. Roman Kagan (4): v2v: drop useless forced gc v2v: consolidate virtio-win file copying v2v: copy virtio drivers without guestfs handle leak v2v: reuse main guestfs for virtio win drivers iso v2v/convert_windows.ml | 184 ++++++++++++++++++++-------------------- v2v/utils.ml | 224 ++++++++++++++++--------------------------------- v2v/v2v.ml | 8 -- 3 files changed, 163 insertions(+), 253 deletions(-) -- 2.4.3

Show replies by date

Roman Kagan

Monday, 10 August Mon, 10 Aug

10:55 a.m.

New subject: [PATCH 1/4] v2v: drop useless forced gc

Forced gc meant, according to the comment, to prevent the leak of the auxiliary guestfs handle (and thus an extra qemu instance) to the data copy stage of the v2v conversion, doesn't actually achieve its goal: it's easy to check that in reality that extra qemu instance survives till the end of v2v execution. So drop it; an alternative scheme will be proposed in the followup patches. Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- v2v/v2v.ml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/v2v/v2v.ml b/v2v/v2v.ml index 242f129..7c47ea0 100644 --- a/v2v/v2v.ml +++ b/v2v/v2v.ml @@ -320,14 +320,6 @@ let rec main () = if verbose () then printf "%s%!" (string_of_target_buses target_buses); - (* Force a GC here, to ensure that we're using the minimum resources - * as we go into the copy stage. The particular reason is that - * Windows conversion may have opened a second libguestfs handle - * pointing to the virtio-win ISO, which is only closed when the - * handle is GC'd. - *) - Gc.compact (); - let delete_target_on_exit = ref true in let targets = -- 2.4.3

Roman Kagan

10:55 a.m.

New subject: [PATCH 2/4] v2v: consolidate virtio-win file copying

Copy the appropriate driver files from the virtio-win storage into %SYSTEMROOT%\Drivers\VirtIO once they are discovered, and stick to using those copies later on. This way the knowledge of where the drivers come from originally is consolidated in one place, so the lifetime of the associated entities becomes easier to control (to be implemented in followup patches). Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- v2v/convert_windows.ml | 128 ++++++++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 72 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index 9e83df8..26609f2 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -231,12 +231,9 @@ echo uninstalling Xen PV driver with Not_found -> () - and install_virtio_drivers root current_cs = - (* Copy the virtio drivers to the guest. *) - let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in - g#mkdir_p driverdir; + and copy_virtio_drivers driverdir = + (* Copy the matching drivers to the driverdir. *) - (* Load the list of drivers available. *) let drivers = find_virtio_win_drivers virtio_win in (* Filter out only drivers matching the current guest. *) @@ -260,73 +257,60 @@ echo uninstalling Xen PV driver flush stdout ); - match drivers with - | [] -> - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch inspect.i_product_variant - virtio_win; - ( IDE, RTL8139, Cirrus ) - - | drivers -> - (* Can we install the block driver? *) - let block : guestcaps_block_type = - try - let viostor_sys_file = - List.find - (fun { vwd_filename = filename } -> filename = "viostor.sys") - drivers in - (* Get the actual file contents of the .sys file. *) - let content = viostor_sys_file.vwd_get_contents () in - let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in - let target = g#case_sensitive_path target in - g#write target content; - add_viostor_to_critical_device_database root current_cs; - Virtio_blk - with Not_found -> - warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - IDE in - - (* Can we install the virtio-net driver? *) - let net : guestcaps_net_type = - if not (List.exists - (fun { vwd_filename = filename } -> filename = "netkvm.inf") - drivers) then ( - warning (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - RTL8139 - ) - else - (* It will be installed at firstboot. *) - Virtio_net in - - (* Can we install the QXL driver? *) - let video : guestcaps_video_type = - if not (List.exists - (fun { vwd_filename = filename } -> filename = "qxl.inf") - drivers) then ( - warning (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use standard VGA.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - Cirrus - ) - else - (* It will be installed at firstboot. *) - QXL in - - (* Copy all the drivers to the driverdir. They will be - * installed at firstboot. - *) - List.iter ( - fun driver -> - let content = driver.vwd_get_contents () in - g#write (driverdir // driver.vwd_filename) content - ) drivers; - - (block, net, video) + List.iter ( + fun driver -> + let content = driver.vwd_get_contents () in + g#write (driverdir // driver.vwd_filename) content + ) drivers + + and install_virtio_drivers root current_cs = + (* Copy the virtio drivers to the guest. *) + let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in + g#mkdir_p driverdir; + + copy_virtio_drivers driverdir; + + (* Can we install the block driver? *) + let block : guestcaps_block_type = + let source = driverdir // "viostor.sys" in + if (g#exists source) then ( + let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in + let target = g#case_sensitive_path target in + g#cp source target; + add_viostor_to_critical_device_database root current_cs; + Virtio_blk + ) else ( + warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + IDE + ) in + + (* Can we install the virtio-net driver? *) + let net : guestcaps_net_type = + if not (g#exists (driverdir // "netkvm.inf")) then ( + warning (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + RTL8139 + ) + else + (* It will be installed at firstboot. *) + Virtio_net in + + (* Can we install the QXL driver? *) + let video : guestcaps_video_type = + if not (g#exists (driverdir // "qxl.inf")) then ( + warning (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use standard VGA.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + Cirrus + ) + else + (* It will be installed at firstboot. *) + QXL in + + (block, net, video) and add_viostor_to_critical_device_database root current_cs = let { i_major_version = major; i_minor_version = minor; -- 2.4.3

Richard W.M. Jones

Tuesday, 11 August Tue, 11 Aug

1:04 p.m.

New subject: [PATCH 2/4] v2v: consolidate virtio-win file copying

On Mon, Aug 10, 2015 at 06:55:30PM +0300, Roman Kagan wrote:

...

This looks good to me (once I used the 'git diff -w' option to show the true changes). So likely ACK. 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/

Roman Kagan

Monday, 10 August Mon, 10 Aug

10:55 a.m.

New subject: [PATCH 3/4] v2v: copy virtio drivers without guestfs handle leak

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(a)virtuozzo.com> --- v2v/convert_windows.ml | 77 ++++++++++------- v2v/utils.ml | 224 ++++++++++++++++--------------------------------- 2 files changed, 118 insertions(+), 183 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index 26609f2..b6e6c62 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -233,35 +233,54 @@ echo uninstalling Xen PV driver and copy_virtio_drivers driverdir = (* Copy the matching drivers to the driverdir. *) - - let drivers = find_virtio_win_drivers virtio_win in - - (* Filter out only drivers matching the current guest. *) - let drivers = - List.filter ( - fun { vwd_os_arch = arch; - vwd_os_major = os_major; vwd_os_minor = os_minor; - vwd_os_variant = os_variant } -> - arch = inspect.i_arch && - os_major = inspect.i_major_version && - os_minor = inspect.i_minor_version && - (match os_variant with - | Vwd_client -> inspect.i_product_variant = "Client" - | Vwd_not_client -> inspect.i_product_variant <> "Client" - | Vwd_any_variant -> true) - ) drivers in - - if verbose () then ( - printf "virtio-win driver files matching this guest:\n"; - List.iter print_virtio_win_driver_file drivers; - flush stdout - ); - - List.iter ( - fun driver -> - let content = driver.vwd_get_contents () in - g#write (driverdir // driver.vwd_filename) content - ) drivers + 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 ( + let source = virtio_win // path in + let target = driverdir // (String.lowercase + (Filename.basename path)) in + if verbose () then + printf "Copying virtio driver bits: 'host:%s' -> '%s'\n" + source target; + + g#write target (read_whole_file source) + ) + ) paths + ) + else if is_regular_file virtio_win then ( + try + let g2 = new Guestfs.guestfs () in + if trace () then g2#set_trace true; + if verbose () then g2#set_verbose true; + g2#add_drive_opts virtio_win ~readonly:true; + g2#launch (); + let vio_root = "/" in + g2#mount_ro "/dev/sda" vio_root; + let paths = g2#find vio_root in + Array.iter ( + fun path -> + let source = vio_root // path in + 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 + let target = driverdir // (String.lowercase + (Filename.basename path)) in + if verbose () then + printf "Copying virtio driver bits: '%s:%s' -> '%s'\n" + virtio_win path target; + + g#write target (g2#read_file source) + ) paths; + g2#close() + with Guestfs.Error msg -> + error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg + ) and install_virtio_drivers root current_cs = (* Copy the virtio drivers to the guest. *) diff --git a/v2v/utils.ml b/v2v/utils.ml index 4e6befc..7db4448 100644 --- a/v2v/utils.ml +++ b/v2v/utils.ml @@ -110,161 +110,77 @@ let find_uefi_firmware guest_arch = in loop files -(* Find virtio-win driver files from an unpacked or mounted virtio-win - * directory, or from a virtio-win.iso file. The location of drivers - varies between releases of virtio-win and also across Fedora and - RHEL so try to be robust to changes. - *) -type virtio_win_driver_file = { - (* Base filename, eg. "netkvm.sys". Always lowercase. *) - vwd_filename : string; - (* Return the contents of this file. *) - vwd_get_contents : unit -> string; - - (* Various fields that classify this driver: *) - - vwd_os_major : int; (* Windows version. *) - vwd_os_minor : int; - vwd_os_variant : vwd_os_variant; - vwd_os_arch : string; (* Architecture, eg "i386", "x86_64". *) - vwd_extension : string; (* File extension (lowercase), eg. "sys" "inf"*) - - (* Original source of file (for debugging only). *) - vwd_original_source : string; -} -and vwd_os_variant = Vwd_client | Vwd_not_client | Vwd_any_variant - -let print_virtio_win_driver_file vwd = - printf "%s [%d,%d,%s,%s,%s] from %s\n" - vwd.vwd_filename - vwd.vwd_os_major vwd.vwd_os_minor - (match vwd.vwd_os_variant with - | Vwd_client -> "client" | Vwd_not_client -> "not-client" - | Vwd_any_variant -> "any") - vwd.vwd_os_arch - vwd.vwd_extension - vwd.vwd_original_source - -let find_virtio_win_drivers virtio_win = - let is_regular_file path = (* NB: follows symlinks. *) - try (Unix.stat path).Unix.st_kind = Unix.S_REG - with Unix.Unix_error _ -> false - in - - let files = - 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.map ( - fun path -> - let abs_path = virtio_win // path in - (path, abs_path, - Filename.basename path, - fun () -> read_whole_file abs_path) - ) paths - ) - else if is_regular_file virtio_win then ( - try - let g = new Guestfs.guestfs () in - if trace () then g#set_trace true; - if verbose () then g#set_verbose true; - g#add_drive_opts virtio_win ~readonly:true; - g#launch (); - g#mount_ro "/dev/sda" "/"; - let paths = g#find "/" in - let paths = Array.to_list paths in - let paths = List.map ((^) "/") paths in - let paths = List.filter (g#is_file ~followsymlinks:false) paths in - List.map ( - fun path -> - let basename = - match last_part_of path '/' with - | Some x -> x - | None -> - error "v2v/find_virtio_win_drivers: missing '/' in %s" path in - (path, sprintf "%s:%s" virtio_win path, - basename, - fun () -> g#read_file path) - ) paths - with Guestfs.Error msg -> - error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg - ) - else [] in +let is_regular_file path = (* NB: follows symlinks. *) + try (Unix.stat path).Unix.st_kind = Unix.S_REG + with Unix.Unix_error _ -> false - let files = - filter_map ( - fun (path, original_source, basename, get_contents) -> - try - (* Lowercased path, since the ISO may contain upper or lowercase - * path elements. XXX This won't work if paths contain non-ASCII. - *) - let lc_path = String.lowercase path in - let lc_basename = String.lowercase basename in - - let extension = - match last_part_of lc_basename '.' with - | Some x -> x - | None -> - error "v2v/find_virtio_win_drivers: missing '.' in %s" - lc_basename in - - (* Skip files without specific extensions. *) - if extension <> "cat" && extension <> "inf" && - extension <> "pdb" && extension <> "sys" then - raise Not_found; - - (* Using the full path, work out what version of Windows - * this driver is for. Paths can be things like: - * "NetKVM/2k12R2/amd64/netkvm.sys" or - * "./drivers/amd64/Win2012R2/netkvm.sys". - * Note we check lowercase paths. - *) - let pathelem elem = string_find lc_path ("/" ^ elem ^ "/") >= 0 in - let arch = - if pathelem "x86" || pathelem "i386" then "i386" - else if pathelem "amd64" then "x86_64" - else raise Not_found in - let os_major, os_minor, os_variant = - if pathelem "xp" || pathelem "winxp" then - (5, 1, Vwd_any_variant) - else if pathelem "2k3" || pathelem "win2003" then - (5, 2, Vwd_any_variant) - else if pathelem "vista" then - (6, 0, Vwd_client) - else if pathelem "2k8" || pathelem "win2008" then - (6, 0, Vwd_not_client) - else if pathelem "w7" || pathelem "win7" then - (6, 1, Vwd_client) - else if pathelem "2k8r2" || pathelem "win2008r2" then - (6, 1, Vwd_not_client) - else if pathelem "w8" || pathelem "win8" then - (6, 2, Vwd_client) - else if pathelem "2k12" || pathelem "win2012" then - (6, 2, Vwd_not_client) - else if pathelem "w8.1" || pathelem "win8.1" then - (6, 3, Vwd_client) - else if pathelem "2k12r2" || pathelem "win2012r2" then - (6, 3, Vwd_not_client) - else if pathelem "w10" || pathelem "win10" then - (10, 0, Vwd_client) - else - raise Not_found in - - Some { - vwd_filename = lc_basename; - vwd_get_contents = get_contents; - vwd_os_major = os_major; - vwd_os_minor = os_minor; - vwd_os_variant = os_variant; - vwd_os_arch = arch; - vwd_extension = extension; - vwd_original_source = original_source; - } - - with Not_found -> None - ) files in - - files +(* 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 = + try + (* Lowercased path, since the ISO may contain upper or lowercase path + * elements. XXX This won't work if paths contain non-ASCII. + *) + let lc_path = String.lowercase path in + let lc_basename = Filename.basename path in + + let extension = + match last_part_of lc_basename '.' with + | Some x -> x + | None -> raise Not_found + in + + (* Skip files without specific extensions. *) + let extensions = ["cat"; "inf"; "pdb"; "sys"] in + if (not (List.mem extension extensions)) then raise Not_found; + + (* Using the full path, work out what version of Windows + * this driver is for. Paths can be things like: + * "NetKVM/2k12R2/amd64/netkvm.sys" or + * "./drivers/amd64/Win2012R2/netkvm.sys". + * Note we check lowercase paths. + *) + let pathelem elem = string_find lc_path ("/" ^ elem ^ "/") >= 0 in + let p_arch = + if pathelem "x86" || pathelem "i386" then "i386" + else if pathelem "amd64" then "x86_64" + else raise Not_found in + + let is_client os_variant = os_variant = "Client" + and not_client os_variant = os_variant <> "Client" + and any_variant os_variant = true in + let p_os_major, p_os_minor, match_os_variant = + if pathelem "xp" || pathelem "winxp" then + (5, 1, any_variant) + else if pathelem "2k3" || pathelem "win2003" then + (5, 2, any_variant) + else if pathelem "vista" then + (6, 0, is_client) + else if pathelem "2k8" || pathelem "win2008" then + (6, 0, not_client) + else if pathelem "w7" || pathelem "win7" then + (6, 1, is_client) + else if pathelem "2k8r2" || pathelem "win2008r2" then + (6, 1, not_client) + else if pathelem "w8" || pathelem "win8" then + (6, 2, is_client) + else if pathelem "2k12" || pathelem "win2012" then + (6, 2, not_client) + else if pathelem "w8.1" || pathelem "win8.1" then + (6, 3, is_client) + else if pathelem "2k12r2" || pathelem "win2012r2" then + (6, 3, not_client) + else if pathelem "w10" || pathelem "win10" then + (10, 0, is_client) + else + raise Not_found in + + arch = p_arch && os_major = p_os_major && os_minor = p_os_minor && + match_os_variant os_variant + + with Not_found -> false let compare_app2_versions app1 app2 = let i = compare app1.Guestfs.app2_epoch app2.Guestfs.app2_epoch in -- 2.4.3

Richard W.M. Jones

Tuesday, 11 August Tue, 11 Aug

1:07 p.m.

New subject: [PATCH 3/4] v2v: copy virtio drivers without guestfs handle leak

On Mon, Aug 10, 2015 at 06:55:31PM +0300, Roman Kagan wrote:

...

And this looks good too. It's basically just a code move. Difficult patch to follow, even with -w (not your fault of course - if only we had better tools!) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

Roman Kagan

Monday, 10 August Mon, 10 Aug

10:55 a.m.

New subject: [PATCH 4/4] v2v: reuse main guestfs for virtio win drivers iso

If we're given a ISO image as the source of virtio windows drivers, reuse the same guestfs handle that is open for accessing the guest being inspected, rather than creating a new one. This is lighter-weight and more convenient, and, given the support for hot-inserting/ejecting an ISO image in libguestfs, appears no less reliable. Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- v2v/convert_windows.ml | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index b6e6c62..51bbd8c 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -254,18 +254,15 @@ echo uninstalling Xen PV driver ) else if is_regular_file virtio_win then ( try - let g2 = new Guestfs.guestfs () in - if trace () then g2#set_trace true; - if verbose () then g2#set_verbose true; - g2#add_drive_opts virtio_win ~readonly:true; - g2#launch (); - let vio_root = "/" in - g2#mount_ro "/dev/sda" vio_root; - let paths = g2#find vio_root in + let label = "viowin" in + g#add_drive_opts virtio_win ~readonly:true ~label:label; + let vio_root = (g#mkdtemp (driverdir // "isoXXXXXX")) in + g#mount_ro ("/dev/disk/guestfs" // label) vio_root; + let paths = g#find vio_root in Array.iter ( fun path -> let source = vio_root // path in - if ((g2#is_file source ~followsymlinks:false) && + if ((g#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 @@ -275,9 +272,11 @@ echo uninstalling Xen PV driver printf "Copying virtio driver bits: '%s:%s' -> '%s'\n" virtio_win path target; - g#write target (g2#read_file source) + g#cp source target ) paths; - g2#close() + g#umount vio_root; + g#remove_drive label; + g#rmdir vio_root with Guestfs.Error msg -> error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg ) -- 2.4.3

Roman Kagan

Thursday, 1 October Thu, 1 Oct

10:04 a.m.

On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote:

...

I just realized that, although (at least two of) these patches got "likely ACK", they are not in the libguestfs tree. The patches still apply to master as of today. Is there anything I can do to get them merged? Thanks, Roman.

Richard W.M. Jones

10:09 a.m.

On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote:

...

On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote: > Libguestfs supports passing an ISO image as a source of virtio windows > drivers to v2v. > > That support, however, looks too heavy-weight: in order to access those > drivers, a separate guestfs handle is created (and thus a new emulator > process is started), which runs until v2v completes. > > This series attempts to make it simpler and lighter-weight, by making > the relevant code more local, and by hot-adding the image into the main > guestfs handle. > > Roman Kagan (4): > v2v: drop useless forced gc > v2v: consolidate virtio-win file copying > v2v: copy virtio drivers without guestfs handle leak > v2v: reuse main guestfs for virtio win drivers iso > > v2v/convert_windows.ml | 184 ++++++++++++++++++++-------------------- > v2v/utils.ml | 224 ++++++++++++++++--------------------------------- > v2v/v2v.ml | 8 -- > 3 files changed, 163 insertions(+), 253 deletions(-) I just realized that, although (at least two of) these patches got "likely ACK", they are not in the libguestfs tree. The patches still apply to master as of today. Is there anything I can do to get them merged?

What happened was I wanted to look at why the inner handle never got GC'd. And I did have a look at that, couldn't work out for the life of me why, and gave up. I'm going to look at the two patches again, so give me a bit. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

Richard W.M. Jones

10:22 a.m.

On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote:

...

Actually better to post them again so I can review them again. I spotted a few problems - There's a missing pair of parentheses around the then clause in: g2#mount_ro "/dev/sda" vio_root; let paths = g2#find vio_root in Array.iter ( fun path -> let source = vio_root // path in 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 ^^^ which makes me think this code can't possibly work. - Calling String.lowercase is unsafe (because the function is just broken in OCaml) and unnecessary. Just remove it. - (Stylistic) You don't need parentheses around if conditionals. - (Stylistic) You don't need parentheses around the arguments of && - (Stylistic) You don't need parentheses around the arguments of // 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

Roman Kagan

11:09 a.m.

On Thu, Oct 01, 2015 at 04:22:14PM +0100, Richard W.M. Jones wrote:

...

On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote: > On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote: > > Libguestfs supports passing an ISO image as a source of virtio windows > > drivers to v2v. > > > > That support, however, looks too heavy-weight: in order to access those > > drivers, a separate guestfs handle is created (and thus a new emulator > > process is started), which runs until v2v completes. > > > > This series attempts to make it simpler and lighter-weight, by making > > the relevant code more local, and by hot-adding the image into the main > > guestfs handle. > > > > Roman Kagan (4): > > v2v: drop useless forced gc > > v2v: consolidate virtio-win file copying > > v2v: copy virtio drivers without guestfs handle leak > > v2v: reuse main guestfs for virtio win drivers iso > > > > v2v/convert_windows.ml | 184 ++++++++++++++++++++-------------------- > > v2v/utils.ml | 224 ++++++++++++++++--------------------------------- > > v2v/v2v.ml | 8 -- > > 3 files changed, 163 insertions(+), 253 deletions(-) > > I just realized that, although (at least two of) these patches got > "likely ACK", they are not in the libguestfs tree. > > The patches still apply to master as of today. > > Is there anything I can do to get them merged? Actually better to post them again so I can review them again.

OK will do.

...

I spotted a few problems - There's a missing pair of parentheses around the then clause in: g2#mount_ro "/dev/sda" vio_root; let paths = g2#find vio_root in Array.iter ( fun path -> let source = vio_root // path in 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 ^^^ which makes me think this code can't possibly work.

Indeed. It failed neither in compilation nor in tests, though. Looks like I need to patch v2v/test-v2v-windows-conversion.sh, too. Roman.

Roman Kagan

Monday, 5 October Mon, 5 Oct

10:52 a.m.

On Thu, Oct 01, 2015 at 07:09:02PM +0300, Roman Kagan wrote:

...

On Thu, Oct 01, 2015 at 04:22:14PM +0100, Richard W.M. Jones wrote: > On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote: > > On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote: > > > Libguestfs supports passing an ISO image as a source of virtio windows > > > drivers to v2v. > > > > > > That support, however, looks too heavy-weight: in order to access those > > > drivers, a separate guestfs handle is created (and thus a new emulator > > > process is started), which runs until v2v completes. > > > > > > This series attempts to make it simpler and lighter-weight, by making > > > the relevant code more local, and by hot-adding the image into the main > > > guestfs handle. > > > > > > Roman Kagan (4): > > > v2v: drop useless forced gc > > > v2v: consolidate virtio-win file copying > > > v2v: copy virtio drivers without guestfs handle leak > > > v2v: reuse main guestfs for virtio win drivers iso > > > > > > v2v/convert_windows.ml | 184 ++++++++++++++++++++-------------------- > > > v2v/utils.ml | 224 ++++++++++++++++--------------------------------- > > > v2v/v2v.ml | 8 -- > > > 3 files changed, 163 insertions(+), 253 deletions(-) > > > > I just realized that, although (at least two of) these patches got > > "likely ACK", they are not in the libguestfs tree. > > > > The patches still apply to master as of today. > > > > Is there anything I can do to get them merged? > > Actually better to post them again so I can review them again. OK will do. > I spotted a few problems > > - There's a missing pair of parentheses around the then clause in: > > g2#mount_ro "/dev/sda" vio_root; > let paths = g2#find vio_root in > Array.iter ( > fun path -> > let source = vio_root // path in > 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 > ^^^ > > which makes me think this code can't possibly work. Indeed. It failed neither in compilation nor in tests, though. Looks like I need to patch v2v/test-v2v-windows-conversion.sh, too.

When I replied I didn't already remember for certain how I verified that the code did achieve its goal; however now that I have a test for it (submitted today, needs fixing the stylistic comments but working otherwise) I claim that it passes with this code, i.e. all the drivers do get copied into the guest upon conversion. So this code does work as intended; being totally clueless in OCaml I'd appreciate being explained -- how?

...

> - Calling String.lowercase is unsafe (because the function is just > broken in OCaml) and unnecessary. Just remove it.

I can't figure out why it's unnecessary and how to remove it. The only place where String.lowercase is called is in match_vio_path_with_os() which is basically unchanged in the patch, it's only factored out into a separate function. It tries to do case-insensitive comparison of the path elements to constant strings; is there a way to do it other than downcasing both sides of the comparison? Thanks, Roman.

Richard W.M. Jones

11:11 a.m.

On Mon, Oct 05, 2015 at 06:52:55PM +0300, Roman Kagan wrote:

...

On Thu, Oct 01, 2015 at 07:09:02PM +0300, Roman Kagan wrote: > On Thu, Oct 01, 2015 at 04:22:14PM +0100, Richard W.M. Jones wrote: > > On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote: > > > On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote: > > > > Libguestfs supports passing an ISO image as a source of virtio windows > > > > drivers to v2v. > > > > > > > > That support, however, looks too heavy-weight: in order to access those > > > > drivers, a separate guestfs handle is created (and thus a new emulator > > > > process is started), which runs until v2v completes. > > > > > > > > This series attempts to make it simpler and lighter-weight, by making > > > > the relevant code more local, and by hot-adding the image into the main > > > > guestfs handle. > > > > > > > > Roman Kagan (4): > > > > v2v: drop useless forced gc > > > > v2v: consolidate virtio-win file copying > > > > v2v: copy virtio drivers without guestfs handle leak > > > > v2v: reuse main guestfs for virtio win drivers iso > > > > > > > > v2v/convert_windows.ml | 184 ++++++++++++++++++++-------------------- > > > > v2v/utils.ml | 224 ++++++++++++++++--------------------------------- > > > > v2v/v2v.ml | 8 -- > > > > 3 files changed, 163 insertions(+), 253 deletions(-) > > > > > > I just realized that, although (at least two of) these patches got > > > "likely ACK", they are not in the libguestfs tree. > > > > > > The patches still apply to master as of today. > > > > > > Is there anything I can do to get them merged? > > > > Actually better to post them again so I can review them again. > > OK will do. > > > I spotted a few problems > > > > - There's a missing pair of parentheses around the then clause in: > > > > g2#mount_ro "/dev/sda" vio_root; > > let paths = g2#find vio_root in > > Array.iter ( > > fun path -> > > let source = vio_root // path in > > 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 > > ^^^ > > > > which makes me think this code can't possibly work. > > Indeed. It failed neither in compilation nor in tests, though. Looks > like I need to patch v2v/test-v2v-windows-conversion.sh, too. When I replied I didn't already remember for certain how I verified that the code did achieve its goal; however now that I have a test for it (submitted today, needs fixing the stylistic comments but working otherwise) I claim that it passes with this code, i.e. all the drivers do get copied into the guest upon conversion. So this code does work as intended; being totally clueless in OCaml I'd appreciate being explained -- how?

It's like in C. The following two if statements do different things: if foo then ( stmt_1; stmt_2; ) if foo then stmt_1; stmt_2; The second one runs stmt_2 unconditionally.

...

> > - Calling String.lowercase is unsafe (because the function is just > > broken in OCaml) and unnecessary. Just remove it. I can't figure out why it's unnecessary and how to remove it. The only place where String.lowercase is called is in match_vio_path_with_os() which is basically unchanged in the patch, it's only factored out into a separate function. It tries to do case-insensitive comparison of the path elements to constant strings; is there a way to do it other than downcasing both sides of the comparison?

Oh I see - yes it's broken upstream too :-( (See the comment "XXX This won't work if paths contain non-ASCII.") Since String.lowercase assumes the string is ISO-8859-1, it could actually corrupt UTF-8 paths. As usual we're in a hard place here because we don't want to add dependencies on external libraries like Camomile (https://github.com/yoriyuki/Camomile) which would be the obvious choice, but no one has written a working replacement for mllib/common_utils.ml. Whatever you do, make sure it's obvious that it's still broken and needs to be fixed. 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

Richard W.M. Jones

Tuesday, 6 October Tue, 6 Oct

7:32 a.m.

On Mon, Oct 05, 2015 at 05:11:52PM +0100, Richard W.M. Jones wrote:

...

If you have a look at the patch series I just posted: https://www.redhat.com/archives/libguestfs/2015-October/msg00068.html You can now use String.lowercase_ascii which is safe. In fact you won't be able to use String.lowercase any longer since that patch series hides the function. 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

Roman Kagan

7:57 a.m.

On Mon, Oct 05, 2015 at 05:11:52PM +0100, Richard W.M. Jones wrote:

...

On Mon, Oct 05, 2015 at 06:52:55PM +0300, Roman Kagan wrote: > On Thu, Oct 01, 2015 at 07:09:02PM +0300, Roman Kagan wrote: > > On Thu, Oct 01, 2015 at 04:22:14PM +0100, Richard W.M. Jones wrote: > > > On Thu, Oct 01, 2015 at 06:04:03PM +0300, Roman Kagan wrote: > > > > On Mon, Aug 10, 2015 at 06:55:28PM +0300, Roman Kagan wrote: > > > > > Libguestfs supports passing an ISO image as a source of virtio windows > > > > > drivers to v2v. > > > > > > > > > > That support, however, looks too heavy-weight: in order to access those > > > > > drivers, a separate guestfs handle is created (and thus a new emulator > > > > > process is started), which runs until v2v completes. > > > > > > > > > > This series attempts to make it simpler and lighter-weight, by making > > > > > the relevant code more local, and by hot-adding the image into the main > > > > > guestfs handle. > > > > > > > > > > Roman Kagan (4): > > > > > v2v: drop useless forced gc > > > > > v2v: consolidate virtio-win file copying > > > > > v2v: copy virtio drivers without guestfs handle leak > > > > > v2v: reuse main guestfs for virtio win drivers iso > > > > > > > > > > v2v/convert_windows.ml | 184 ++++++++++++++++++++-------------------- > > > > > v2v/utils.ml | 224 ++++++++++++++++--------------------------------- > > > > > v2v/v2v.ml | 8 -- > > > > > 3 files changed, 163 insertions(+), 253 deletions(-) > > > > > > > > I just realized that, although (at least two of) these patches got > > > > "likely ACK", they are not in the libguestfs tree. > > > > > > > > The patches still apply to master as of today. > > > > > > > > Is there anything I can do to get them merged? > > > > > > Actually better to post them again so I can review them again. > > > > OK will do. > > > > > I spotted a few problems > > > > > > - There's a missing pair of parentheses around the then clause in: > > > > > > g2#mount_ro "/dev/sda" vio_root; > > > let paths = g2#find vio_root in > > > Array.iter ( > > > fun path -> > > > let source = vio_root // path in > > > 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 > > > ^^^ > > > > > > which makes me think this code can't possibly work. > > > > Indeed. It failed neither in compilation nor in tests, though. Looks > > like I need to patch v2v/test-v2v-windows-conversion.sh, too. > > When I replied I didn't already remember for certain how I verified that > the code did achieve its goal; however now that I have a test for it > (submitted today, needs fixing the stylistic comments but working > otherwise) I claim that it passes with this code, i.e. all the drivers > do get copied into the guest upon conversion. > > So this code does work as intended; being totally clueless in OCaml I'd > appreciate being explained -- how? It's like in C. The following two if statements do different things: if foo then ( stmt_1; stmt_2; ) if foo then stmt_1; stmt_2; The second one runs stmt_2 unconditionally.

Looks right... The problem is that the statement which runs unconditionally is g#cp source target where, in case the condition evaluates to false, "target" is undefined, and "source" is anything returned from g#find vio_root, including directories. I thought that would case an exception one way or another, but it doesn't...

...

> > > - Calling String.lowercase is unsafe (because the function is just > > > broken in OCaml) and unnecessary. Just remove it. > > I can't figure out why it's unnecessary and how to remove it. The only > place where String.lowercase is called is in match_vio_path_with_os() > which is basically unchanged in the patch, it's only factored out into a > separate function. It tries to do case-insensitive comparison of the > path elements to constant strings; is there a way to do it other than > downcasing both sides of the comparison? Oh I see - yes it's broken upstream too :-( (See the comment "XXX This won't work if paths contain non-ASCII.") Since String.lowercase assumes the string is ISO-8859-1, it could actually corrupt UTF-8 paths. As usual we're in a hard place here because we don't want to add dependencies on external libraries like Camomile (https://github.com/yoriyuki/Camomile) which would be the obvious choice, but no one has written a working replacement for mllib/common_utils.ml. Whatever you do, make sure it's obvious that it's still broken and needs to be fixed.

The XXX comment was kept unmodified, and is now sitting at the top of this function. So I guess I can consider this concern addressed, can I? I just came across another problem in the last patch of the series, though: I accidentally configured libguestfs to use direct backend, and immediately discovered that reusing the main guestfs handle doesn't work with it due to the lack of support for hotplugging drives. So I'll need to reconsider it somehow (perhaps try with hotplug and fall back to creating a new handle on exception). Thanks, Roman.

Richard W.M. Jones

8:12 a.m.

On Tue, Oct 06, 2015 at 03:57:51PM +0300, Roman Kagan wrote:

...

Something is never going to be undefined in OCaml. There's no such thing. At this point it'd be really useful if you could repost the rebased patches, so I can see the code that we're talking about.

...

I just came across another problem in the last patch of the series, though: I accidentally configured libguestfs to use direct backend, and immediately discovered that reusing the main guestfs handle doesn't work with it due to the lack of support for hotplugging drives. So I'll need to reconsider it somehow (perhaps try with hotplug and fall back to creating a new handle on exception).

Why don't we just use the second handle. The real issue is why the handle isn't being cleaned up by the GC, which may reveal some deeper bug somewhere. I only had a brief look at this. 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/

Roman Kagan

8:33 a.m.

On Tue, Oct 06, 2015 at 02:12:03PM +0100, Richard W.M. Jones wrote:

...

On Tue, Oct 06, 2015 at 03:57:51PM +0300, Roman Kagan wrote: > Looks right... The problem is that the statement which runs > unconditionally is > > g#cp source target > > where, in case the condition evaluates to false, "target" is undefined, > and "source" is anything returned from g#find vio_root, including > directories. I thought that would case an exception one way or another, > but it doesn't... Something is never going to be undefined in OCaml. There's no such thing.

I guess you mean that it would trigger a compile error; it didn't, and it overall worked as if the parentheses were there. OK it doesn't really matter, I'll stick in the parentheses anyway; no point digging into OCaml peculiarities.

...

> I just came across another problem in the last patch of the series, > though: I accidentally configured libguestfs to use direct backend, and > immediately discovered that reusing the main guestfs handle doesn't work > with it due to the lack of support for hotplugging drives. So I'll need > to reconsider it somehow (perhaps try with hotplug and fall back to > creating a new handle on exception). Why don't we just use the second handle.

Well, because it's resource-intensive and looks like an overkill for a simple job of parsing an iso image. Not that critical, of course, and we can drop the 4th patch altogether, if you think it's not worth while. Roman.

Richard W.M. Jones

8:51 a.m.

On Tue, Oct 06, 2015 at 02:12:03PM +0100, Richard W.M. Jones wrote:

...

Why don't we just use the second handle. The real issue is why the handle isn't being cleaned up by the GC, which may reveal some deeper bug somewhere. I only had a brief look at this.

I'm able to reproduce this in a minimal program. Still investigating. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

Richard W.M. Jones

10:08 a.m.

On Tue, Oct 06, 2015 at 02:51:46PM +0100, Richard W.M. Jones wrote:

...

On Tue, Oct 06, 2015 at 02:12:03PM +0100, Richard W.M. Jones wrote: > Why don't we just use the second handle. The real issue is why the > handle isn't being cleaned up by the GC, which may reveal some deeper > bug somewhere. I only had a brief look at this. I'm able to reproduce this in a minimal program. Still investigating.

Fixed, see: https://www.redhat.com/archives/libguestfs/2015-October/msg00082.html and I've tested that virt-v2v with this patch series closes the second handle when Gc.compact () is called. Thanks for noticing this bug! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

Roman Kagan

Tuesday, 13 October Tue, 13 Oct

7:50 a.m.

New subject: [PATCH v2 0/4] v2v: simplify driver copying from virtio-win iso

Libguestfs supports passing an ISO image as a source of virtio windows drivers to v2v. That support, however, looks too heavy-weight: in order to access those drivers, a separate guestfs handle is created (and thus a new emulator process is started), which runs until v2v triggers garbage collection. This series attempts to make it simpler and lighter-weight, by making the relevant code more local, and by hot-adding the image into the main guestfs handle if supported by the backend. Roman Kagan (4): v2v: consolidate virtio-win file copying v2v: copy virtio drivers without guestfs handle leak v2v: drop useless forced gc v2v: reuse main guestfs for virtio win drivers iso --- changes since v1: - rebased to latest master (in particular, updated usage of string functions) - add missing and remove excessive parentheses - reorder patches - preserve support for backends with no hot-add capability v2v/convert_windows.ml | 206 ++++++++++++++++++++++++++--------------------- v2v/utils.ml | 212 +++++++++++++++---------------------------------- v2v/v2v.ml | 8 -- 3 files changed, 179 insertions(+), 247 deletions(-) -- 2.4.3

Roman Kagan

7:50 a.m.

New subject: [PATCH v2 1/4] v2v: consolidate virtio-win file copying

Copy the appropriate driver files from the virtio-win storage into %SYSTEMROOT%\Drivers\VirtIO once they are discovered, and stick to using those copies later on. This way the knowledge of where the drivers come from originally is consolidated in one place, so the lifetime of the associated entities becomes easier to control (to be implemented in followup patches). Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- changes since v1: - former patch #2 v2v/convert_windows.ml | 128 ++++++++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 72 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index d1aef36..55213aa 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -271,12 +271,9 @@ echo uninstalling Xen PV driver with Not_found -> () - and install_virtio_drivers root current_cs = - (* Copy the virtio drivers to the guest. *) - let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in - g#mkdir_p driverdir; + and copy_virtio_drivers driverdir = + (* Copy the matching drivers to the driverdir. *) - (* Load the list of drivers available. *) let drivers = find_virtio_win_drivers virtio_win in (* Filter out only drivers matching the current guest. *) @@ -300,73 +297,60 @@ echo uninstalling Xen PV driver flush stdout ); - match drivers with - | [] -> - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch inspect.i_product_variant - virtio_win; - ( IDE, RTL8139, Cirrus ) - - | drivers -> - (* Can we install the block driver? *) - let block : guestcaps_block_type = - try - let viostor_sys_file = - List.find - (fun { vwd_filename = filename } -> filename = "viostor.sys") - drivers in - (* Get the actual file contents of the .sys file. *) - let content = viostor_sys_file.vwd_get_contents () in - let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in - let target = g#case_sensitive_path target in - g#write target content; - add_viostor_to_critical_device_database root current_cs; - Virtio_blk - with Not_found -> - warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - IDE in - - (* Can we install the virtio-net driver? *) - let net : guestcaps_net_type = - if not (List.exists - (fun { vwd_filename = filename } -> filename = "netkvm.inf") - drivers) then ( - warning (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - RTL8139 - ) - else - (* It will be installed at firstboot. *) - Virtio_net in - - (* Can we install the QXL driver? *) - let video : guestcaps_video_type = - if not (List.exists - (fun { vwd_filename = filename } -> filename = "qxl.inf") - drivers) then ( - warning (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use standard VGA.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - Cirrus - ) - else - (* It will be installed at firstboot. *) - QXL in - - (* Copy all the drivers to the driverdir. They will be - * installed at firstboot. - *) - List.iter ( - fun driver -> - let content = driver.vwd_get_contents () in - g#write (driverdir // driver.vwd_filename) content - ) drivers; - - (block, net, video) + List.iter ( + fun driver -> + let content = driver.vwd_get_contents () in + g#write (driverdir // driver.vwd_filename) content + ) drivers + + and install_virtio_drivers root current_cs = + (* Copy the virtio drivers to the guest. *) + let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in + g#mkdir_p driverdir; + + copy_virtio_drivers driverdir; + + (* Can we install the block driver? *) + let block : guestcaps_block_type = + let source = driverdir // "viostor.sys" in + if (g#exists source) then ( + let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in + let target = g#case_sensitive_path target in + g#cp source target; + add_viostor_to_critical_device_database root current_cs; + Virtio_blk + ) else ( + warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + IDE + ) in + + (* Can we install the virtio-net driver? *) + let net : guestcaps_net_type = + if not (g#exists (driverdir // "netkvm.inf")) then ( + warning (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + RTL8139 + ) + else + (* It will be installed at firstboot. *) + Virtio_net in + + (* Can we install the QXL driver? *) + let video : guestcaps_video_type = + if not (g#exists (driverdir // "qxl.inf")) then ( + warning (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use standard VGA.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + Cirrus + ) + else + (* It will be installed at firstboot. *) + QXL in + + (block, net, video) and add_viostor_to_critical_device_database root current_cs = let { i_major_version = major; i_minor_version = minor; -- 2.4.3

Richard W.M. Jones

9:16 a.m.

New subject: [PATCH v2 1/4] v2v: consolidate virtio-win file copying

On Tue, Oct 13, 2015 at 03:50:51PM +0300, Roman Kagan wrote:

...

- match drivers with - | [] -> - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch inspect.i_product_variant - virtio_win; - ( IDE, RTL8139, Cirrus )

I believe this warning gets lost in this patch. To get it back, let's make 'copy_virtio_drivers' returns a true/false indication ('true' meaning that at least some drivers were copied), and the caller code in 'install_virtio_drivers' should check this, and either do the code above, or continue with the code below.

...

+ copy_virtio_drivers driverdir; + + (* Can we install the block driver? *) + let block : guestcaps_block_type = + let source = driverdir // "viostor.sys" in + if (g#exists source) then (

Don't need parentheses around if-conditions. The rest of the patch looks good with these two changes. 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

Roman Kagan

Wednesday, 14 October Wed, 14 Oct

6:26 a.m.

New subject: [PATCH v2 1/4] v2v: consolidate virtio-win file copying

On Tue, Oct 13, 2015 at 03:16:27PM +0100, Richard W.M. Jones wrote:

...

On Tue, Oct 13, 2015 at 03:50:51PM +0300, Roman Kagan wrote: > > - match drivers with > - | [] -> > - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") > - inspect.i_major_version inspect.i_minor_version > - inspect.i_arch inspect.i_product_variant > - virtio_win; > - ( IDE, RTL8139, Cirrus ) I believe this warning gets lost in this patch. To get it back, let's make 'copy_virtio_drivers' returns a true/false indication ('true' meaning that at least some drivers were copied), and the caller code in 'install_virtio_drivers' should check this, and either do the code above, or continue with the code below.

Is there any difference between not being able to locate some of the virtio drivers or all of them? With this patch, you'll get an individual warning for every one of the missing drivers. I don't see why we need an extra case for "all drivers missing" (for any particular value of "all"). Roman.

Richard W.M. Jones

6:34 a.m.

New subject: [PATCH v2 1/4] v2v: consolidate virtio-win file copying

On Wed, Oct 14, 2015 at 02:26:22PM +0300, Roman Kagan wrote:

...

On Tue, Oct 13, 2015 at 03:16:27PM +0100, Richard W.M. Jones wrote: > On Tue, Oct 13, 2015 at 03:50:51PM +0300, Roman Kagan wrote: > > > > - match drivers with > > - | [] -> > > - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") > > - inspect.i_major_version inspect.i_minor_version > > - inspect.i_arch inspect.i_product_variant > > - virtio_win; > > - ( IDE, RTL8139, Cirrus ) > > I believe this warning gets lost in this patch. > > To get it back, let's make 'copy_virtio_drivers' returns a true/false > indication ('true' meaning that at least some drivers were copied), > and the caller code in 'install_virtio_drivers' should check this, and > either do the code above, or continue with the code below. Is there any difference between not being able to locate some of the virtio drivers or all of them?

Yes - if there are no drivers located at all, it indicates some more fundamental failure such as the user pointing to the wrong ISO. In any case, if it were right to remove this warning, then it should be done in a separate commit. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

Roman Kagan

Tuesday, 13 October Tue, 13 Oct

7:50 a.m.

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

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(a)virtuozzo.com> --- changes since v1: - former patch #3 - updated usage of string functions v2v/convert_windows.ml | 78 +++++++++++------- v2v/utils.ml | 212 +++++++++++++++---------------------------------- 2 files changed, 113 insertions(+), 177 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index 55213aa..cfa5474 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -273,35 +273,55 @@ echo uninstalling Xen PV driver and copy_virtio_drivers driverdir = (* Copy the matching drivers to the driverdir. *) - - let drivers = find_virtio_win_drivers virtio_win in - - (* Filter out only drivers matching the current guest. *) - let drivers = - List.filter ( - fun { vwd_os_arch = arch; - vwd_os_major = os_major; vwd_os_minor = os_minor; - vwd_os_variant = os_variant } -> - arch = inspect.i_arch && - os_major = inspect.i_major_version && - os_minor = inspect.i_minor_version && - (match os_variant with - | Vwd_client -> inspect.i_product_variant = "Client" - | Vwd_not_client -> inspect.i_product_variant <> "Client" - | Vwd_any_variant -> true) - ) drivers in - - if verbose () then ( - printf "virtio-win driver files matching this guest:\n"; - List.iter print_virtio_win_driver_file drivers; - flush stdout - ); - - List.iter ( - fun driver -> - let content = driver.vwd_get_contents () in - g#write (driverdir // driver.vwd_filename) content - ) drivers + 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 ( + let source = virtio_win // path in + let target = driverdir // (String.lowercase_ascii + (Filename.basename path)) in + if verbose () then + printf "Copying virtio driver bits: 'host:%s' -> '%s'\n" + source target; + + g#write target (read_whole_file source) + ) + ) paths + ) + else if is_regular_file virtio_win then ( + try + let g2 = new Guestfs.guestfs () in + if trace () then g2#set_trace true; + if verbose () then g2#set_verbose true; + g2#add_drive_opts virtio_win ~readonly:true; + g2#launch (); + let vio_root = "/" in + g2#mount_ro "/dev/sda" vio_root; + let paths = g2#find vio_root in + Array.iter ( + fun path -> + let source = vio_root // path in + 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 ( + let target = driverdir // (String.lowercase_ascii + (Filename.basename path)) in + if verbose () then + printf "Copying virtio driver bits: '%s:%s' -> '%s'\n" + virtio_win path target; + + g#write target (g2#read_file source) + ) + ) paths; + g2#close() + with Guestfs.Error msg -> + error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg + ) and install_virtio_drivers root current_cs = (* Copy the virtio drivers to the guest. *) diff --git a/v2v/utils.ml b/v2v/utils.ml index 2998d90..3d0bec8 100644 --- a/v2v/utils.ml +++ b/v2v/utils.ml @@ -159,161 +159,77 @@ let find_uefi_firmware guest_arch = in loop files -(* Find virtio-win driver files from an unpacked or mounted virtio-win - * directory, or from a virtio-win.iso file. The location of drivers - varies between releases of virtio-win and also across Fedora and - RHEL so try to be robust to changes. - *) -type virtio_win_driver_file = { - (* Base filename, eg. "netkvm.sys". Always lowercase. *) - vwd_filename : string; - (* Return the contents of this file. *) - vwd_get_contents : unit -> string; - - (* Various fields that classify this driver: *) - - vwd_os_major : int; (* Windows version. *) - vwd_os_minor : int; - vwd_os_variant : vwd_os_variant; - vwd_os_arch : string; (* Architecture, eg "i386", "x86_64". *) - vwd_extension : string; (* File extension (lowercase), eg. "sys" "inf"*) +let is_regular_file path = (* NB: follows symlinks. *) + try (Unix.stat path).Unix.st_kind = Unix.S_REG + with Unix.Unix_error _ -> false - (* Original source of file (for debugging only). *) - vwd_original_source : string; -} -and vwd_os_variant = Vwd_client | Vwd_not_client | Vwd_any_variant - -let print_virtio_win_driver_file vwd = - printf "%s [%d,%d,%s,%s,%s] from %s\n" - vwd.vwd_filename - vwd.vwd_os_major vwd.vwd_os_minor - (match vwd.vwd_os_variant with - | Vwd_client -> "client" | Vwd_not_client -> "not-client" - | Vwd_any_variant -> "any") - vwd.vwd_os_arch - vwd.vwd_extension - vwd.vwd_original_source - -let find_virtio_win_drivers virtio_win = - let is_regular_file path = (* NB: follows symlinks. *) - try (Unix.stat path).Unix.st_kind = Unix.S_REG - with Unix.Unix_error _ -> false - in - - let files = - 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.map ( - fun path -> - let abs_path = virtio_win // path in - (path, abs_path, - Filename.basename path, - fun () -> read_whole_file abs_path) - ) paths - ) - else if is_regular_file virtio_win then ( - try - let g = new Guestfs.guestfs () in - g#set_identifier "virtio_win"; - if trace () then g#set_trace true; - if verbose () then g#set_verbose true; - g#add_drive_opts virtio_win ~readonly:true; - g#launch (); - g#mount_ro "/dev/sda" "/"; - let paths = g#find "/" in - let paths = Array.to_list paths in - let paths = List.map ((^) "/") paths in - let paths = List.filter (g#is_file ~followsymlinks:false) paths in - List.map ( - fun path -> - let basename = - match last_part_of path '/' with - | Some x -> x - | None -> - error "v2v/find_virtio_win_drivers: missing '/' in %s" path in - (path, sprintf "%s:%s" virtio_win path, - basename, - fun () -> g#read_file path) - ) paths - with Guestfs.Error msg -> - error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg - ) - else [] in - - let files = - filter_map ( - fun (path, original_source, basename, get_contents) -> - try - (* Lowercased path, since the ISO may contain upper or lowercase - * path elements. - *) - let lc_path = String.lowercase_ascii path in - let lc_basename = String.lowercase_ascii basename 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 = + try + (* Lowercased path, since the ISO may contain upper or lowercase path + * elements. + *) + let lc_path = String.lowercase_ascii path in + let lc_basename = Filename.basename path in - let extension = - match last_part_of lc_basename '.' with - | Some x -> x - | None -> raise Not_found - in + let extension = + match last_part_of lc_basename '.' with + | Some x -> x + | None -> raise Not_found + in - (* Skip files without specific extensions. *) - if extension <> "cat" && extension <> "inf" && - extension <> "pdb" && extension <> "sys" then - raise Not_found; + (* Skip files without specific extensions. *) + let extensions = ["cat"; "inf"; "pdb"; "sys"] in + if (not (List.mem extension extensions)) then raise Not_found; - (* Using the full path, work out what version of Windows - * this driver is for. Paths can be things like: - * "NetKVM/2k12R2/amd64/netkvm.sys" or - * "./drivers/amd64/Win2012R2/netkvm.sys". - * Note we check lowercase paths. - *) - let pathelem elem = String.find lc_path ("/" ^ elem ^ "/") >= 0 in - let arch = - if pathelem "x86" || pathelem "i386" then "i386" - else if pathelem "amd64" then "x86_64" - else raise Not_found in - let os_major, os_minor, os_variant = - if pathelem "xp" || pathelem "winxp" then - (5, 1, Vwd_any_variant) - else if pathelem "2k3" || pathelem "win2003" then - (5, 2, Vwd_any_variant) - else if pathelem "vista" then - (6, 0, Vwd_client) - else if pathelem "2k8" || pathelem "win2008" then - (6, 0, Vwd_not_client) - else if pathelem "w7" || pathelem "win7" then - (6, 1, Vwd_client) - else if pathelem "2k8r2" || pathelem "win2008r2" then - (6, 1, Vwd_not_client) - else if pathelem "w8" || pathelem "win8" then - (6, 2, Vwd_client) - else if pathelem "2k12" || pathelem "win2012" then - (6, 2, Vwd_not_client) - else if pathelem "w8.1" || pathelem "win8.1" then - (6, 3, Vwd_client) - else if pathelem "2k12r2" || pathelem "win2012r2" then - (6, 3, Vwd_not_client) - else if pathelem "w10" || pathelem "win10" then - (10, 0, Vwd_client) - else - raise Not_found in + (* Using the full path, work out what version of Windows + * this driver is for. Paths can be things like: + * "NetKVM/2k12R2/amd64/netkvm.sys" or + * "./drivers/amd64/Win2012R2/netkvm.sys". + * Note we check lowercase paths. + *) + let pathelem elem = String.find lc_path ("/" ^ elem ^ "/") >= 0 in + let p_arch = + if pathelem "x86" || pathelem "i386" then "i386" + else if pathelem "amd64" then "x86_64" + else raise Not_found in - Some { - vwd_filename = lc_basename; - vwd_get_contents = get_contents; - vwd_os_major = os_major; - vwd_os_minor = os_minor; - vwd_os_variant = os_variant; - vwd_os_arch = arch; - vwd_extension = extension; - vwd_original_source = original_source; - } + let is_client os_variant = os_variant = "Client" + and not_client os_variant = os_variant <> "Client" + and any_variant os_variant = true in + let p_os_major, p_os_minor, match_os_variant = + if pathelem "xp" || pathelem "winxp" then + (5, 1, any_variant) + else if pathelem "2k3" || pathelem "win2003" then + (5, 2, any_variant) + else if pathelem "vista" then + (6, 0, is_client) + else if pathelem "2k8" || pathelem "win2008" then + (6, 0, not_client) + else if pathelem "w7" || pathelem "win7" then + (6, 1, is_client) + else if pathelem "2k8r2" || pathelem "win2008r2" then + (6, 1, not_client) + else if pathelem "w8" || pathelem "win8" then + (6, 2, is_client) + else if pathelem "2k12" || pathelem "win2012" then + (6, 2, not_client) + else if pathelem "w8.1" || pathelem "win8.1" then + (6, 3, is_client) + else if pathelem "2k12r2" || pathelem "win2012r2" then + (6, 3, not_client) + else if pathelem "w10" || pathelem "win10" then + (10, 0, is_client) + else + raise Not_found in - with Not_found -> None - ) files in + arch = p_arch && os_major = p_os_major && os_minor = p_os_minor && + match_os_variant os_variant - files + with Not_found -> false let compare_app2_versions app1 app2 = let i = compare app1.Guestfs.app2_epoch app2.Guestfs.app2_epoch in -- 2.4.3

Richard W.M. Jones

9:27 a.m.

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

On Tue, Oct 13, 2015 at 03:50:52PM +0300, Roman Kagan wrote:

...

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

Richard W.M. Jones

Wednesday, 14 October Wed, 14 Oct

3:41 a.m.

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

Some more problems: On Tue, Oct 13, 2015 at 03:50:52PM +0300, Roman Kagan wrote:

...

+ let g2 = new Guestfs.guestfs () in + if trace () then g2#set_trace true; + if verbose () then g2#set_verbose true; + g2#add_drive_opts virtio_win ~readonly:true; + g2#launch ();

This ends up dropping the change made in commit bd0b52d08e1fbffc947eae9274ae30cf3a17737e. Please add it back.

...

+ let vio_root = "/" in + g2#mount_ro "/dev/sda" vio_root; + let paths = g2#find vio_root in + Array.iter ( + fun path -> + let source = vio_root // path in + 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 (

Just pass the 'inspect' struct here, rather than the individual fields. It's less code and more type safe. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

Roman Kagan

Tuesday, 13 October Tue, 13 Oct

7:50 a.m.

New subject: [PATCH v2 3/4] v2v: drop useless forced gc

Now that virtio driver copying is localized in a single function and the auxiliary guestfs handle doesn't leak outside of it (and thus an extra qemu instance doesn't survive beyond that function's runtime) there's no need in the forced GC which used to trigger closing of that handle and termination of that qemu instance. Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- changes since v1: - former patch #1 v2v/v2v.ml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/v2v/v2v.ml b/v2v/v2v.ml index 4d621b8..fe16131 100644 --- a/v2v/v2v.ml +++ b/v2v/v2v.ml @@ -319,14 +319,6 @@ let rec main () = if verbose () then printf "%s%!" (string_of_target_buses target_buses); - (* Force a GC here, to ensure that we're using the minimum resources - * as we go into the copy stage. The particular reason is that - * Windows conversion may have opened a second libguestfs handle - * pointing to the virtio-win ISO, which is only closed when the - * handle is GC'd. - *) - Gc.compact (); - let delete_target_on_exit = ref true in let targets = -- 2.4.3

Roman Kagan

7:50 a.m.

New subject: [PATCH v2 4/4] v2v: reuse main guestfs for virtio win drivers iso

If we're given an ISO image as the source of virtio windows drivers, and the backend supports hot-adding drives, reuse the same guestfs handle that is open for accessing the guest being inspected, rather than creating a new one (which would run another virtual machine instance and is fairly resource-intensive). Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- changes since v1: - updated usage of string functions - add missing and remove excessive parentheses - preserve support for backends with no hot-add capability v2v/convert_windows.ml | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index cfa5474..5fd8bb6 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -294,13 +294,25 @@ echo uninstalling Xen PV driver ) else if is_regular_file virtio_win then ( try - let g2 = new Guestfs.guestfs () in - if trace () then g2#set_trace true; - if verbose () then g2#set_verbose true; - g2#add_drive_opts virtio_win ~readonly:true; - g2#launch (); - let vio_root = "/" in - g2#mount_ro "/dev/sda" vio_root; + let label = "viowin" in + let vio_root = "/virtio-win-iso" in + let g2 = + (* not all backends support hot-adding drives; create an extra + * guestfs handle in that case *) + try + g#add_drive_opts virtio_win ~readonly:true ~label:label; + g + with Guestfs.Error msg -> ( + let g2 = new Guestfs.guestfs () in + if trace () then g2#set_trace true; + if verbose () then g2#set_verbose true; + g2#add_drive_opts virtio_win ~readonly:true ~label:label; + g2#launch (); + g2 + ) in + + g2#mkmountpoint vio_root; + g2#mount_ro ("/dev/disk/guestfs" // label) vio_root; let paths = g2#find vio_root in Array.iter ( fun path -> @@ -315,10 +327,18 @@ echo uninstalling Xen PV driver printf "Copying virtio driver bits: '%s:%s' -> '%s'\n" virtio_win path target; - g#write target (g2#read_file source) + if (g2 == g) then + g#cp source target + else + g#write target (g2#read_file source) ) ) paths; - g2#close() + g2#umount vio_root; + g2#rmmountpoint vio_root; + if (g2 == g) then + g2#remove_drive label + else + g2#close() with Guestfs.Error msg -> error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg ) -- 2.4.3

Richard W.M. Jones

8:36 a.m.

New subject: [PATCH v2 4/4] v2v: reuse main guestfs for virtio win drivers iso

On Tue, Oct 13, 2015 at 03:50:54PM +0300, Roman Kagan wrote:

...

I really don't like this specific patch. Hotplugging is not very reliable in my experience. The patch tries to work out if hotplugging is going to work by calling guestfs_add_drive and taking an alternate path, but I'm not so convinced that would work on other architectures (eg. hotplugging on aarch64 is in various states between not working at all and flaky). Are you observing any benefit with this patch? v2v conversion takes a long time - I can't believe the extra overhead of creating a handle could make much difference. I'll look at the other patches soon. 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

Roman Kagan

9:07 a.m.

New subject: [PATCH v2 4/4] v2v: reuse main guestfs for virtio win drivers iso

On Tue, Oct 13, 2015 at 02:36:10PM +0100, Richard W.M. Jones wrote:

...

On Tue, Oct 13, 2015 at 03:50:54PM +0300, Roman Kagan wrote: > If we're given an ISO image as the source of virtio windows drivers, and > the backend supports hot-adding drives, reuse the same guestfs handle > that is open for accessing the guest being inspected, rather than > creating a new one (which would run another virtual machine instance and > is fairly resource-intensive). I really don't like this specific patch. Hotplugging is not very reliable in my experience. The patch tries to work out if hotplugging is going to work by calling guestfs_add_drive and taking an alternate path, but I'm not so convinced that would work on other architectures (eg. hotplugging on aarch64 is in various states between not working at all and flaky). Are you observing any benefit with this patch? v2v conversion takes a long time - I can't believe the extra overhead of creating a handle could make much difference.

Frankly, I don't have convincing numbers to justify this patch. In my measurements with test conversions on an overwise idle workstation gives on the order of 5% gain where statistical variance is just a little less; OTOH LIBGUESTFS_BACKEND=direct gives statistically significant 30-40% improvement :) We've been thinking of doing mass VM conversion where an extra qemu instance for every v2v operation would create significant memory and io overhead but I don't have a proof of that. In view of hot-add being unstable as you say, I'm fine with dropping this patch altogether. Thanks, Roman.

Roman Kagan

Wednesday, 14 October Wed, 14 Oct

11:55 a.m.

New subject: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

Libguestfs supports passing an ISO image as a source of virtio windows drivers to v2v. This series attempts to make it simpler and better scoped. Roman Kagan (3): v2v: consolidate virtio-win file copying v2v: copy virtio drivers without guestfs handle leak v2v: drop useless forced gc --- changes since v2: - drop patch 4 (reuse of the master guestfs handle for hot-adding the ISO image) - more excessive parentheses removed - revert the removal of a warning when no virtio drivers matching the guest OS are found - revert the undesired removal of guestfs handle identifier assignment changes since v1: - rebased to latest master (in particular, updated usage of string functions) - add missing and remove excessive parentheses - reorder patches - preserve support for backends with no hot-add capability v2v/convert_windows.ml | 192 +++++++++++++++++++++++--------------------- v2v/utils.ml | 213 +++++++++++++++---------------------------------- v2v/v2v.ml | 8 -- 3 files changed, 167 insertions(+), 246 deletions(-) -- 2.4.3

Roman Kagan

11:55 a.m.

New subject: [PATCH v3 1/3] v2v: consolidate virtio-win file copying

Copy the appropriate driver files from the virtio-win storage into %SYSTEMROOT%\Drivers\VirtIO once they are discovered, and stick to using those copies later on. This way the knowledge of where the drivers come from originally is consolidated in one place, so the lifetime of the associated entities becomes easier to control (to be implemented in followup patches). Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- changes since v2: - more excessive parentheses removed - revert the removal of a warning when no virtio drivers matching the guest OS are found changes since v1: - former patch #2 v2v/convert_windows.ml | 139 +++++++++++++++++++++++-------------------------- 1 file changed, 66 insertions(+), 73 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index d1aef36..c103d7c 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -271,12 +271,9 @@ echo uninstalling Xen PV driver with Not_found -> () - and install_virtio_drivers root current_cs = - (* Copy the virtio drivers to the guest. *) - let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in - g#mkdir_p driverdir; - - (* Load the list of drivers available. *) + and copy_virtio_drivers driverdir = + (* Copy the matching drivers to the driverdir; return true if any have been + * copied *) let drivers = find_virtio_win_drivers virtio_win in (* Filter out only drivers matching the current guest. *) @@ -300,73 +297,69 @@ echo uninstalling Xen PV driver flush stdout ); - match drivers with - | [] -> - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch inspect.i_product_variant - virtio_win; - ( IDE, RTL8139, Cirrus ) - - | drivers -> - (* Can we install the block driver? *) - let block : guestcaps_block_type = - try - let viostor_sys_file = - List.find - (fun { vwd_filename = filename } -> filename = "viostor.sys") - drivers in - (* Get the actual file contents of the .sys file. *) - let content = viostor_sys_file.vwd_get_contents () in - let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in - let target = g#case_sensitive_path target in - g#write target content; - add_viostor_to_critical_device_database root current_cs; - Virtio_blk - with Not_found -> - warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - IDE in - - (* Can we install the virtio-net driver? *) - let net : guestcaps_net_type = - if not (List.exists - (fun { vwd_filename = filename } -> filename = "netkvm.inf") - drivers) then ( - warning (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - RTL8139 - ) - else - (* It will be installed at firstboot. *) - Virtio_net in - - (* Can we install the QXL driver? *) - let video : guestcaps_video_type = - if not (List.exists - (fun { vwd_filename = filename } -> filename = "qxl.inf") - drivers) then ( - warning (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use standard VGA.") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win; - Cirrus - ) - else - (* It will be installed at firstboot. *) - QXL in - - (* Copy all the drivers to the driverdir. They will be - * installed at firstboot. - *) - List.iter ( - fun driver -> - let content = driver.vwd_get_contents () in - g#write (driverdir // driver.vwd_filename) content - ) drivers; - - (block, net, video) + let ret = ref false in + List.iter ( + fun driver -> + let content = driver.vwd_get_contents () in + g#write (driverdir // driver.vwd_filename) content; + ret := true + ) drivers; + !ret + + and install_virtio_drivers root current_cs = + (* Copy the virtio drivers to the guest. *) + let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in + g#mkdir_p driverdir; + + if not (copy_virtio_drivers driverdir) then ( + warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") + inspect.i_major_version inspect.i_minor_version inspect.i_arch + inspect.i_product_variant virtio_win; + ( IDE, RTL8139, Cirrus ) + ) + else ( + (* Can we install the block driver? *) + let block : guestcaps_block_type = + let source = driverdir // "viostor.sys" in + if g#exists source then ( + let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in + let target = g#case_sensitive_path target in + g#cp source target; + add_viostor_to_critical_device_database root current_cs; + Virtio_blk + ) else ( + warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + IDE + ) in + + (* Can we install the virtio-net driver? *) + let net : guestcaps_net_type = + if not (g#exists (driverdir // "netkvm.inf")) then ( + warning (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + RTL8139 + ) + else + (* It will be installed at firstboot. *) + Virtio_net in + + (* Can we install the QXL driver? *) + let video : guestcaps_video_type = + if not (g#exists (driverdir // "qxl.inf")) then ( + warning (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use standard VGA.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win; + Cirrus + ) + else + (* It will be installed at firstboot. *) + QXL in + + (block, net, video) + ) and add_viostor_to_critical_device_database root current_cs = let { i_major_version = major; i_minor_version = minor; -- 2.4.3

Roman Kagan

11:55 a.m.

New subject: [PATCH v3 2/3] v2v: copy virtio drivers without guestfs handle leak

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(a)virtuozzo.com> --- changes since v2: - more excessive parentheses removed - revert the removal of a warning when no virtio drivers matching the guest OS are found - revert the undesired removal of guestfs handle identifier assignment changes since v1: - former patch #3 - updated usage of string functions v2v/convert_windows.ml | 77 +++++++++++------- v2v/utils.ml | 213 +++++++++++++++---------------------------------- 2 files changed, 113 insertions(+), 177 deletions(-) diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml index c103d7c..ed2542e 100644 --- a/v2v/convert_windows.ml +++ b/v2v/convert_windows.ml @@ -274,36 +274,55 @@ echo uninstalling Xen PV driver and copy_virtio_drivers driverdir = (* Copy the matching drivers to the driverdir; return true if any have been * copied *) - let drivers = find_virtio_win_drivers virtio_win in - - (* Filter out only drivers matching the current guest. *) - let drivers = - List.filter ( - fun { vwd_os_arch = arch; - vwd_os_major = os_major; vwd_os_minor = os_minor; - vwd_os_variant = os_variant } -> - arch = inspect.i_arch && - os_major = inspect.i_major_version && - os_minor = inspect.i_minor_version && - (match os_variant with - | Vwd_client -> inspect.i_product_variant = "Client" - | Vwd_not_client -> inspect.i_product_variant <> "Client" - | Vwd_any_variant -> true) - ) drivers in - - if verbose () then ( - printf "virtio-win driver files matching this guest:\n"; - List.iter print_virtio_win_driver_file drivers; - flush stdout - ); - let ret = ref false in - List.iter ( - fun driver -> - let content = driver.vwd_get_contents () in - g#write (driverdir // driver.vwd_filename) content; - ret := true - ) drivers; + 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 then ( + let source = virtio_win // path in + let target = driverdir // + String.lowercase_ascii (Filename.basename path) in + if verbose () then + printf "Copying virtio driver bits: 'host:%s' -> '%s'\n" + source target; + + g#write target (read_whole_file source); + ret := true + ) + ) paths + ) + else if is_regular_file virtio_win then ( + try + let g2 = new Guestfs.guestfs () in + g#set_identifier "virtio_win"; + if trace () then g2#set_trace true; + if verbose () then g2#set_verbose true; + g2#add_drive_opts virtio_win ~readonly:true; + g2#launch (); + let vio_root = "/" in + g2#mount_ro "/dev/sda" vio_root; + let paths = g2#find vio_root in + Array.iter ( + fun path -> + let source = vio_root // path in + if g2#is_file source ~followsymlinks:false && + match_vio_path_with_os path inspect then ( + let target = driverdir // + String.lowercase_ascii (Filename.basename path) in + if verbose () then + printf "Copying virtio driver bits: '%s:%s' -> '%s'\n" + virtio_win path target; + + g#write target (g2#read_file source); + ret := true + ) + ) paths; + g2#close() + with Guestfs.Error msg -> + error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg + ); !ret and install_virtio_drivers root current_cs = diff --git a/v2v/utils.ml b/v2v/utils.ml index 2998d90..8aaac09 100644 --- a/v2v/utils.ml +++ b/v2v/utils.ml @@ -159,161 +159,78 @@ let find_uefi_firmware guest_arch = in loop files -(* Find virtio-win driver files from an unpacked or mounted virtio-win - * directory, or from a virtio-win.iso file. The location of drivers - varies between releases of virtio-win and also across Fedora and - RHEL so try to be robust to changes. - *) -type virtio_win_driver_file = { - (* Base filename, eg. "netkvm.sys". Always lowercase. *) - vwd_filename : string; - (* Return the contents of this file. *) - vwd_get_contents : unit -> string; - - (* Various fields that classify this driver: *) - - vwd_os_major : int; (* Windows version. *) - vwd_os_minor : int; - vwd_os_variant : vwd_os_variant; - vwd_os_arch : string; (* Architecture, eg "i386", "x86_64". *) - vwd_extension : string; (* File extension (lowercase), eg. "sys" "inf"*) +let is_regular_file path = (* NB: follows symlinks. *) + try (Unix.stat path).Unix.st_kind = Unix.S_REG + with Unix.Unix_error _ -> false - (* Original source of file (for debugging only). *) - vwd_original_source : string; -} -and vwd_os_variant = Vwd_client | Vwd_not_client | Vwd_any_variant - -let print_virtio_win_driver_file vwd = - printf "%s [%d,%d,%s,%s,%s] from %s\n" - vwd.vwd_filename - vwd.vwd_os_major vwd.vwd_os_minor - (match vwd.vwd_os_variant with - | Vwd_client -> "client" | Vwd_not_client -> "not-client" - | Vwd_any_variant -> "any") - vwd.vwd_os_arch - vwd.vwd_extension - vwd.vwd_original_source - -let find_virtio_win_drivers virtio_win = - let is_regular_file path = (* NB: follows symlinks. *) - try (Unix.stat path).Unix.st_kind = Unix.S_REG - with Unix.Unix_error _ -> false - in - - let files = - 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.map ( - fun path -> - let abs_path = virtio_win // path in - (path, abs_path, - Filename.basename path, - fun () -> read_whole_file abs_path) - ) paths - ) - else if is_regular_file virtio_win then ( - try - let g = new Guestfs.guestfs () in - g#set_identifier "virtio_win"; - if trace () then g#set_trace true; - if verbose () then g#set_verbose true; - g#add_drive_opts virtio_win ~readonly:true; - g#launch (); - g#mount_ro "/dev/sda" "/"; - let paths = g#find "/" in - let paths = Array.to_list paths in - let paths = List.map ((^) "/") paths in - let paths = List.filter (g#is_file ~followsymlinks:false) paths in - List.map ( - fun path -> - let basename = - match last_part_of path '/' with - | Some x -> x - | None -> - error "v2v/find_virtio_win_drivers: missing '/' in %s" path in - (path, sprintf "%s:%s" virtio_win path, - basename, - fun () -> g#read_file path) - ) paths - with Guestfs.Error msg -> - error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win msg - ) - else [] in - - let files = - filter_map ( - fun (path, original_source, basename, get_contents) -> - try - (* Lowercased path, since the ISO may contain upper or lowercase - * path elements. - *) - let lc_path = String.lowercase_ascii path in - let lc_basename = String.lowercase_ascii basename 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 inspect = + let { i_major_version = os_major; i_minor_version = os_minor; + i_arch = arch; i_product_variant = os_variant } = inspect in + try + (* Lowercased path, since the ISO may contain upper or lowercase path + * elements. *) + let lc_path = String.lowercase_ascii path in + let lc_basename = Filename.basename path in - let extension = - match last_part_of lc_basename '.' with - | Some x -> x - | None -> raise Not_found - in + let extension = + match last_part_of lc_basename '.' with + | Some x -> x + | None -> raise Not_found + in - (* Skip files without specific extensions. *) - if extension <> "cat" && extension <> "inf" && - extension <> "pdb" && extension <> "sys" then - raise Not_found; + (* Skip files without specific extensions. *) + let extensions = ["cat"; "inf"; "pdb"; "sys"] in + if not (List.mem extension extensions) then raise Not_found; - (* Using the full path, work out what version of Windows - * this driver is for. Paths can be things like: - * "NetKVM/2k12R2/amd64/netkvm.sys" or - * "./drivers/amd64/Win2012R2/netkvm.sys". - * Note we check lowercase paths. - *) - let pathelem elem = String.find lc_path ("/" ^ elem ^ "/") >= 0 in - let arch = - if pathelem "x86" || pathelem "i386" then "i386" - else if pathelem "amd64" then "x86_64" - else raise Not_found in - let os_major, os_minor, os_variant = - if pathelem "xp" || pathelem "winxp" then - (5, 1, Vwd_any_variant) - else if pathelem "2k3" || pathelem "win2003" then - (5, 2, Vwd_any_variant) - else if pathelem "vista" then - (6, 0, Vwd_client) - else if pathelem "2k8" || pathelem "win2008" then - (6, 0, Vwd_not_client) - else if pathelem "w7" || pathelem "win7" then - (6, 1, Vwd_client) - else if pathelem "2k8r2" || pathelem "win2008r2" then - (6, 1, Vwd_not_client) - else if pathelem "w8" || pathelem "win8" then - (6, 2, Vwd_client) - else if pathelem "2k12" || pathelem "win2012" then - (6, 2, Vwd_not_client) - else if pathelem "w8.1" || pathelem "win8.1" then - (6, 3, Vwd_client) - else if pathelem "2k12r2" || pathelem "win2012r2" then - (6, 3, Vwd_not_client) - else if pathelem "w10" || pathelem "win10" then - (10, 0, Vwd_client) - else - raise Not_found in + (* Using the full path, work out what version of Windows + * this driver is for. Paths can be things like: + * "NetKVM/2k12R2/amd64/netkvm.sys" or + * "./drivers/amd64/Win2012R2/netkvm.sys". + * Note we check lowercase paths. + *) + let pathelem elem = String.find lc_path ("/" ^ elem ^ "/") >= 0 in + let p_arch = + if pathelem "x86" || pathelem "i386" then "i386" + else if pathelem "amd64" then "x86_64" + else raise Not_found in - Some { - vwd_filename = lc_basename; - vwd_get_contents = get_contents; - vwd_os_major = os_major; - vwd_os_minor = os_minor; - vwd_os_variant = os_variant; - vwd_os_arch = arch; - vwd_extension = extension; - vwd_original_source = original_source; - } + let is_client os_variant = os_variant = "Client" + and not_client os_variant = os_variant <> "Client" + and any_variant os_variant = true in + let p_os_major, p_os_minor, match_os_variant = + if pathelem "xp" || pathelem "winxp" then + (5, 1, any_variant) + else if pathelem "2k3" || pathelem "win2003" then + (5, 2, any_variant) + else if pathelem "vista" then + (6, 0, is_client) + else if pathelem "2k8" || pathelem "win2008" then + (6, 0, not_client) + else if pathelem "w7" || pathelem "win7" then + (6, 1, is_client) + else if pathelem "2k8r2" || pathelem "win2008r2" then + (6, 1, not_client) + else if pathelem "w8" || pathelem "win8" then + (6, 2, is_client) + else if pathelem "2k12" || pathelem "win2012" then + (6, 2, not_client) + else if pathelem "w8.1" || pathelem "win8.1" then + (6, 3, is_client) + else if pathelem "2k12r2" || pathelem "win2012r2" then + (6, 3, not_client) + else if pathelem "w10" || pathelem "win10" then + (10, 0, is_client) + else + raise Not_found in - with Not_found -> None - ) files in + arch = p_arch && os_major = p_os_major && os_minor = p_os_minor && + match_os_variant os_variant - files + with Not_found -> false let compare_app2_versions app1 app2 = let i = compare app1.Guestfs.app2_epoch app2.Guestfs.app2_epoch in -- 2.4.3

Roman Kagan

11:55 a.m.

New subject: [PATCH v3 3/3] v2v: drop useless forced gc

Now that virtio driver copying is localized in a single function and the auxiliary guestfs handle doesn't leak outside of it (and thus an extra qemu instance doesn't survive beyond that function's runtime) there's no need in the forced GC which used to trigger closing of that handle and termination of that qemu instance. Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> --- no changes since v2 changes since v1: - former patch #1 v2v/v2v.ml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/v2v/v2v.ml b/v2v/v2v.ml index 4d621b8..fe16131 100644 --- a/v2v/v2v.ml +++ b/v2v/v2v.ml @@ -319,14 +319,6 @@ let rec main () = if verbose () then printf "%s%!" (string_of_target_buses target_buses); - (* Force a GC here, to ensure that we're using the minimum resources - * as we go into the copy stage. The particular reason is that - * Windows conversion may have opened a second libguestfs handle - * pointing to the virtio-win ISO, which is only closed when the - * handle is GC'd. - *) - Gc.compact (); - let delete_target_on_exit = ref true in let targets = -- 2.4.3

Richard W.M. Jones

Thursday, 15 October Thu, 15 Oct

4:07 a.m.

New subject: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote:

...

Libguestfs supports passing an ISO image as a source of virtio windows drivers to v2v. This series attempts to make it simpler and better scoped.

Looks good (apart from patch 3 - the Gc.compact is necessary!) I'm just running the tests and will push it later if that goes well. Thanks, 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

Roman Kagan

4:27 a.m.

New subject: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones wrote:

...

On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote: > Libguestfs supports passing an ISO image as a source of virtio windows > drivers to v2v. > > This series attempts to make it simpler and better scoped. Looks good (apart from patch 3 - the Gc.compact is necessary!)

Why? As a matter of fact that was the primary reason I even started with this patchset: that Gc.compact was sitting in the middle of the main v2v function and was yet another obstacle in the refactoring I was after. Now that the extra guestfs handle doesn't survive beyond copy_virtio_drivers() what is the need for that explicit GC (which was introduced at the time exactly to clear up that handle in commit 47b5f245bec908f803f0a89c3b1e3166cfe33aad)? Roman.

Richard W.M. Jones

4:40 a.m.

New subject: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

On Thu, Oct 15, 2015 at 12:27:58PM +0300, Roman Kagan wrote:

...

On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones wrote: > On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote: > > Libguestfs supports passing an ISO image as a source of virtio windows > > drivers to v2v. > > > > This series attempts to make it simpler and better scoped. > > Looks good (apart from patch 3 - the Gc.compact is necessary!) Why? As a matter of fact that was the primary reason I even started with this patchset: that Gc.compact was sitting in the middle of the main v2v function and was yet another obstacle in the refactoring I was after. Now that the extra guestfs handle doesn't survive beyond copy_virtio_drivers() what is the need for that explicit GC (which was introduced at the time exactly to clear up that handle in commit 47b5f245bec908f803f0a89c3b1e3166cfe33aad)?

Just because the handle becomes unreachable doesn't mean the GC will immediately collect it. Objects are only required to be finalized when you invoke Gc.full_major (Gc.compact calls Gc.full_major). Previous to these patches, the handle was still unreachable, but Gc.compact didn't free it because of a different problem. That different issue has been fixed by: https://github.com/libguestfs/libguestfs/commit/8bbc5e73cb5b56b5cfbe979ac... In summary, we still need to call Gc.compact before doing the long-running copy operation, to make sure that the resources used by virt-v2v are minimized during the copy. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

Roman Kagan

4:57 a.m.

New subject: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

On Thu, Oct 15, 2015 at 10:40:15AM +0100, Richard W.M. Jones wrote:

...

On Thu, Oct 15, 2015 at 12:27:58PM +0300, Roman Kagan wrote: > On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones wrote: > > On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote: > > > Libguestfs supports passing an ISO image as a source of virtio windows > > > drivers to v2v. > > > > > > This series attempts to make it simpler and better scoped. > > > > Looks good (apart from patch 3 - the Gc.compact is necessary!) > > Why? As a matter of fact that was the primary reason I even started > with this patchset: that Gc.compact was sitting in the middle of the > main v2v function and was yet another obstacle in the refactoring I was > after. > > Now that the extra guestfs handle doesn't survive beyond > copy_virtio_drivers() what is the need for that explicit GC (which was > introduced at the time exactly to clear up that handle in commit > 47b5f245bec908f803f0a89c3b1e3166cfe33aad)? Just because the handle becomes unreachable doesn't mean the GC will immediately collect it. Objects are only required to be finalized when you invoke Gc.full_major (Gc.compact calls Gc.full_major). Previous to these patches, the handle was still unreachable, but Gc.compact didn't free it because of a different problem. That different issue has been fixed by: https://github.com/libguestfs/libguestfs/commit/8bbc5e73cb5b56b5cfbe979ac... In summary, we still need to call Gc.compact before doing the long-running copy operation, to make sure that the resources used by virt-v2v are minimized during the copy.

What resources are you talking about? The only costly resource eater is the auxiliary hypervisor backend instance; it used to be terminated indirectly via this forced GC here (after your fix), but now it's shut down explictly before leaving copy_virtio_drivers(). Anything else that can get collected here is negligible compared to what remains. Roman.

Richard W.M. Jones

5:03 a.m.

New subject: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

On Thu, Oct 15, 2015 at 12:57:04PM +0300, Roman Kagan wrote:

...

Right, now I see what you mean. So in fact I will include your 3rd patch. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

3325

days inactive

3391

days old

guestfs@lists.libguestfs.org

Manage subscription

39 comments

2 participants

tags (0)

participants (2)

Richard W.M. Jones
Roman Kagan

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH 0/4] v2v: simplify driver copying from virtio-win iso