[Libguestfs] [PATCH 3/4] v2v: copy virtio drivers without guestfs handle leak
Richard W.M. Jones
rjones at redhat.com
Tue Aug 11 18:07:39 UTC 2015
On Mon, Aug 10, 2015 at 06:55:31PM +0300, Roman Kagan wrote:
> Refactor copying of virtio windows drivers into the guest so that the
> matching of the drivers to the guest os flavor and copying the files
> happens one next to the other in a single function, and no guestfs
> handle (nor any other irrelevant info) is leaked outside.
>
> Signed-off-by: Roman Kagan <rkagan at virtuozzo.com>
> ---
> 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
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
More information about the Libguestfs
mailing list