On Mon, Nov 08, 2021 at 12:19:15PM +0100, Laszlo Ersek wrote:

On 11/08/21 10:12, Richard W.M. Jones wrote:

...

This was part of the old in-place support. When we add new in-place support we'll do something else, but currently this is dead code so remove it completely.

Note this removes the code for installing the virtio-scsi driver (only ever using virtio-blk). This was also dead code in the current implementation of virt-v2v, but worth remembering in case we want to resurrect this feature in a future version. --- convert/convert.ml | 10 ++--- convert/convert_linux.ml | 24 +++------- convert/convert_linux.mli | 2 +- convert/convert_windows.ml | 4 +- convert/convert_windows.mli | 2 +- convert/windows_virtio.ml | 87 ++++++------------------------------- convert/windows_virtio.mli | 9 +--- lib/types.ml | 20 --------- lib/types.mli | 10 ----- 9 files changed, 30 insertions(+), 138 deletions(-)

This is exactly the kind of enormous patch that I don't trust myself to implement in a project I'm new to. It's difficult to find the boundaries (= where we stop cutting), and also difficult to find everything that becomes unused / dead after the removals. (For example, I had to check that "string_of_video" remained in use, via a different call path.)

I also find reviewing this pretty hard. The patch modifies many (arguably) disparate aspects that I can't *all* keep easily in mind during review. I think I might have attempted a more bottom-up, gradual elimination approach, but that's exactly because I must watch my step very closely. I do think this patch is fine.

TBH I just chopped out every mention of rcaps / requested_guestcaps over breakfast. The compiler is very good at finding mistakes in these kinds of patches because it won't compile something unless all parameters/function exactly match, all matches are complete, etc. Old virt-v2v was written in Perl and that was dreadful for refactoring. Thanks, Rich.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks! Laszlo

...

diff --git a/convert/convert.ml b/convert/convert.ml index 07c7aee90..109e55284 100644 --- a/convert/convert.ml +++ b/convert/convert.ml @@ -230,10 +230,7 @@ helper-v2v-convert V2VDIR

(* Conversion. *) let guestcaps = - let rcaps = (*XXX*) - { rcaps_block_bus = None; rcaps_net_bus = None; rcaps_video = None } in - - do_convert g source inspect keep_serial_console rcaps static_ips in + do_convert g source inspect keep_serial_console static_ips in

g#umount_all ();

@@ -364,7 +361,7 @@ and do_fstrim g inspect = ) fses

(* Conversion. *) -and do_convert g source inspect keep_serial_console rcaps interfaces = +and do_convert g source inspect keep_serial_console interfaces = (match inspect.i_product_name with | "unknown" -> message (f_"Converting the guest to run on KVM") @@ -388,9 +385,8 @@ and do_convert g source inspect keep_serial_console rcaps interfaces = error (f_"virt-v2v is unable to convert this guest type (%s/%s)") inspect.i_type inspect.i_distro in debug "picked conversion module %s" conversion_name; - debug "requested caps: %s" (string_of_requested_guestcaps rcaps); let guestcaps = - convert g source inspect keep_serial_console rcaps interfaces in + convert g source inspect keep_serial_console interfaces in debug "%s" (string_of_guestcaps guestcaps);

(* Did we manage to install virtio drivers? *) diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 057cf9dff..b0b5d916d 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -34,7 +34,7 @@ open Linux_kernels module G = Guestfs

(* The conversion function. *) -let convert (g : G.guestfs) source inspect keep_serial_console rcaps _ = +let convert (g : G.guestfs) source inspect keep_serial_console _ = (*----------------------------------------------------------------------*) (* Inspect the guest first. We already did some basic inspection in * the common v2v.ml code, but that has to deal with generic guests @@ -111,22 +111,12 @@ let convert (g : G.guestfs) source inspect keep_serial_console rcaps _ =

let acpi = supports_acpi () in

- let video = - match rcaps.rcaps_video with - | None -> QXL - | Some video -> video in - let block_type = - match rcaps.rcaps_block_bus with - | None -> if kernel.ki_supports_virtio_blk then Virtio_blk else IDE - | Some block_type -> block_type in - + if kernel.ki_supports_virtio_blk then Virtio_blk else IDE in let net_type = - match rcaps.rcaps_net_bus with - | None -> if kernel.ki_supports_virtio_net then Virtio_net else E1000 - | Some net_type -> net_type in + if kernel.ki_supports_virtio_net then Virtio_net else E1000 in

- configure_display_driver video; + configure_display_driver (); remap_block_devices block_type; configure_kernel_modules block_type net_type; rebuild_initrd kernel; @@ -158,7 +148,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console rcaps _ = let guestcaps = { gcaps_block_bus = block_type; gcaps_net_bus = net_type; - gcaps_video = video; + gcaps_video = QXL; gcaps_virtio_rng = kernel.ki_supports_virtio_rng; gcaps_virtio_balloon = kernel.ki_supports_virtio_balloon; gcaps_isa_pvpanic = kernel.ki_supports_isa_pvpanic; @@ -828,8 +818,8 @@ let convert (g : G.guestfs) source inspect keep_serial_console rcaps _ = else true

- and configure_display_driver video = - let video_driver = match video with QXL -> "qxl" | Cirrus -> "cirrus" in + and configure_display_driver () = + let video_driver = "qxl" in

let updated = ref false in

diff --git a/convert/convert_linux.mli b/convert/convert_linux.mli index 8adc172a0..688a7acea 100644 --- a/convert/convert_linux.mli +++ b/convert/convert_linux.mli @@ -23,5 +23,5 @@ Mint and Kali are supported by this module. *)

val convert : Guestfs.guestfs -> Types.source -> Types.inspect -> - bool -> Types.requested_guestcaps -> Types.static_ip list -> + bool -> Types.static_ip list -> Types.guestcaps diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index f2c1132bf..31e16723b 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -38,7 +38,7 @@ module G = Guestfs * time the Windows VM is booted on KVM. *)

-let convert (g : G.guestfs) _ inspect _ rcaps static_ips = +let convert (g : G.guestfs) _ inspect _ static_ips = (*----------------------------------------------------------------------*) (* Inspect the Windows guest. *)

@@ -469,7 +469,7 @@ if errorlevel 3010 exit /b 0 disable_xenpv_win_drivers reg; disable_prl_drivers reg; disable_autoreboot reg; - Windows_virtio.install_drivers reg inspect rcaps + Windows_virtio.install_drivers reg inspect

and disable_xenpv_win_drivers reg = (* Disable xenpv-win service (RHBZ#809273). *) diff --git a/convert/convert_windows.mli b/convert/convert_windows.mli index d2978d6a4..d3ea3d6ae 100644 --- a/convert/convert_windows.mli +++ b/convert/convert_windows.mli @@ -21,5 +21,5 @@ This module converts a Windows guest to run on KVM. *)

val convert : Guestfs.guestfs -> Types.source -> Types.inspect -> - bool -> Types.requested_guestcaps -> Types.static_ip list -> + bool -> Types.static_ip list -> Types.guestcaps diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml index f843dae2d..08db4ad69 100644 --- a/convert/windows_virtio.ml +++ b/convert/windows_virtio.ml @@ -44,31 +44,16 @@ let viostor_modern_pciid = "VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01" let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00" let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01"

-let rec install_drivers ((g, _) as reg) inspect rcaps = +let rec install_drivers ((g, _) as reg) inspect = (* Copy the virtio drivers to the guest. *) let driverdir = sprintf "%s/Drivers/VirtIO" inspect.i_windows_systemroot in g#mkdir_p driverdir;

if not (copy_drivers g inspect driverdir) then ( - match rcaps with - | { rcaps_block_bus = Some Virtio_blk | Some Virtio_SCSI } - | { rcaps_net_bus = Some Virtio_net } - | { rcaps_video = Some QXL } -> - error (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s") - inspect.i_major_version inspect.i_minor_version inspect.i_arch - inspect.i_product_variant virtio_win - - | { rcaps_block_bus = (Some IDE | None); - rcaps_net_bus = ((Some E1000 | Some RTL8139 | None) as net_type); - rcaps_video = (Some Cirrus | None) } -> 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; - let net_type = - match net_type with - | Some model -> model - | None -> RTL8139 in - (IDE, net_type, Cirrus, false, false, false, false) + (IDE, RTL8139, Cirrus, false, false, false, false) ) else ( (* Can we install the block driver? *) @@ -83,25 +68,14 @@ let rec install_drivers ((g, _) as reg) inspect rcaps = ) filenames ) ) with Not_found -> None in - let has_vioscsi = g#exists (driverdir // "vioscsi.inf") in - match rcaps.rcaps_block_bus, viostor_driver, has_vioscsi with - | Some Virtio_blk, None, _ -> - error (f_"there is no 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 - - | Some Virtio_SCSI, _, false -> - error (f_"there is no vioscsi (virtio SCSI) 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 - - | None, None, _ -> + match viostor_driver with + | None -> warning (f_"there is no 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

- | (Some Virtio_blk | None), Some driver_name, _ -> + | Some driver_name -> (* Block driver needs tweaks to allow booting; the rest is set up by PnP * manager *) let source = driverdir // (driver_name ^ ".sys") in @@ -111,22 +85,7 @@ let rec install_drivers ((g, _) as reg) inspect rcaps = g#cp source target; add_guestor_to_registry reg inspect driver_name viostor_legacy_pciid; add_guestor_to_registry reg inspect driver_name viostor_modern_pciid; - Virtio_blk - - | Some Virtio_SCSI, _, true -> - (* Block driver needs tweaks to allow booting; the rest is set up by PnP - * manager *) - let source = driverdir // "vioscsi.sys" in - let target = sprintf "%s/system32/drivers/vioscsi.sys" - inspect.i_windows_systemroot in - let target = g#case_sensitive_path target in - g#cp source target; - add_guestor_to_registry reg inspect "vioscsi" vioscsi_legacy_pciid; - add_guestor_to_registry reg inspect "vioscsi" vioscsi_modern_pciid; - Virtio_SCSI - - | Some IDE, _, _ -> - IDE in + Virtio_blk in

(* Can we install the virtio-net driver? *) let net : guestcaps_net_type = @@ -135,46 +94,28 @@ let rec install_drivers ((g, _) as reg) inspect rcaps = List.exists ( fun driver_file -> g#exists (driverdir // driver_file) ) filenames in - match rcaps.rcaps_net_bus, has_netkvm with - | Some Virtio_net, false -> - error (f_"there is no virtio network driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win - - | None, false -> + if not has_netkvm 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 - - | (Some Virtio_net | None), true -> - Virtio_net - - | Some net_type, _ -> - net_type in + ) + else + Virtio_net in

(* Can we install the QXL driver? *) let video : guestcaps_video_type = let has_qxl = g#exists (driverdir // "qxl.inf") || g#exists (driverdir // "qxldod.inf") in - match rcaps.rcaps_video, has_qxl with - | Some QXL, false -> - error (f_"there is no QXL driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s") - inspect.i_major_version inspect.i_minor_version - inspect.i_arch virtio_win - - | None, false -> + if not has_qxl 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 a basic VGA display driver.") inspect.i_major_version inspect.i_minor_version inspect.i_arch virtio_win; Cirrus - - | (Some QXL | None), true -> - QXL - - | Some Cirrus, _ -> - Cirrus in + ) + else + QXL in

(* Did we install the miscellaneous drivers? *) let virtio_rng_supported = g#exists (driverdir // "viorng.inf") in diff --git a/convert/windows_virtio.mli b/convert/windows_virtio.mli index 642317b15..4e24625a4 100644 --- a/convert/windows_virtio.mli +++ b/convert/windows_virtio.mli @@ -19,9 +19,9 @@ (** Functions for installing Windows virtio drivers. *)

val install_drivers - : Registry.t -> Types.inspect -> Types.requested_guestcaps -> + : Registry.t -> Types.inspect -> Types.guestcaps_block_type * Types.guestcaps_net_type * Types.guestcaps_video_type * bool * bool * bool * bool -(** [install_drivers reg inspect rcaps] +(** [install_drivers reg inspect] installs virtio drivers from the driver directory or driver ISO into the guest driver directory and updates the registry so that the [viostor.sys] driver gets loaded by Windows at boot. @@ -29,11 +29,6 @@ val install_drivers [reg] is the system hive which is open for writes when this function is called.

- [rcaps] is the set of guest "capabilities" requested by the caller. This - may include the type of the block driver, network driver, and video driver. - install_drivers will adjust its choices based on that information, and - abort if the requested driver wasn't found. - This returns the tuple [(block_driver, net_driver, video_driver, virtio_rng_supported, virtio_ballon_supported, isa_pvpanic_supported)] reflecting what devices are now required by the guest, either diff --git a/lib/types.ml b/lib/types.ml index 72d10d1ec..9be3e6fcd 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -422,11 +422,6 @@ type guestcaps = { gcaps_arch : string; gcaps_acpi : bool; } -and requested_guestcaps = { - rcaps_block_bus : guestcaps_block_type option; - rcaps_net_bus : guestcaps_net_type option; - rcaps_video : guestcaps_video_type option; -} and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_video_type = QXL | Cirrus @@ -463,21 +458,6 @@ gcaps_acpi = %b gcaps.gcaps_arch gcaps.gcaps_acpi

-let string_of_requested_guestcaps rcaps = - sprintf "\ -rcaps_block_bus = %s -rcaps_net_bus = %s -rcaps_video = %s -" (match rcaps.rcaps_block_bus with - | None -> "unspecified" - | Some block_type -> (string_of_block_type block_type)) - (match rcaps.rcaps_net_bus with - | None -> "unspecified" - | Some net_type -> (string_of_net_type net_type)) - (match rcaps.rcaps_video with - | None -> "unspecified" - | Some video -> (string_of_video video)) - type target_buses = { target_virtio_blk_bus : target_bus_slot array; target_ide_bus : target_bus_slot array; diff --git a/lib/types.mli b/lib/types.mli index 0bae445d8..4d4049605 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -282,22 +282,12 @@ type guestcaps = { } (** Guest capabilities after conversion. eg. Was virtio found or installed? *)

-and requested_guestcaps = { - rcaps_block_bus : guestcaps_block_type option; - rcaps_net_bus : guestcaps_net_type option; - rcaps_video : guestcaps_video_type option; -} -(** For [--in-place] conversions, the requested guest capabilities, to - allow the caller to affect conversion choices. [None] = no - preference, use the best available. *) - and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_video_type = QXL | Cirrus and guestcaps_machine = I440FX | Q35 | Virt

val string_of_guestcaps : guestcaps -> string -val string_of_requested_guestcaps : requested_guestcaps -> string

(** {2 Guest buses} *)

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

On 11/08/21 10:12, Richard W.M. Jones wrote:

Since we no longer support requesting virtio-scsi, this constructor was never used. --- convert/convert.ml | 2 +- convert/convert_linux.ml | 9 ++------- convert/target_bus_assignment.ml | 1 - lib/create_ovf.ml | 1 - lib/types.ml | 3 +-- lib/types.mli | 2 +- output/create_json.ml | 1 - output/openstack_image_properties.ml | 7 ------- 8 files changed, 5 insertions(+), 21 deletions(-)

Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks, Laszlo

diff --git a/convert/convert.ml b/convert/convert.ml index 109e55284..2fc618420 100644 --- a/convert/convert.ml +++ b/convert/convert.ml @@ -392,7 +392,7 @@ and do_convert g source inspect keep_serial_console interfaces = (* Did we manage to install virtio drivers? *) if not (quiet ()) then ( match guestcaps.gcaps_block_bus with - | Virtio_blk | Virtio_SCSI -> + | Virtio_blk -> info (f_"This guest has virtio drivers installed.") | IDE -> info (f_"This guest does not have virtio drivers installed.") diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index b0b5d916d..41bc4218e 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -915,12 +915,8 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = (* Update 'alias scsi_hostadapter ...' *) let paths = augeas_modprobe ". =~ regexp('scsi_hostadapter.*')" in (match block_type with - | Virtio_blk | Virtio_SCSI -> - let block_module = - match block_type with - | Virtio_blk -> "virtio_blk" - | Virtio_SCSI -> "virtio_scsi" - | IDE -> assert false in + | Virtio_blk -> + let block_module = "virtio_blk" in

if paths <> [] then ( (* There's only 1 scsi controller in the converted guest. @@ -993,7 +989,6 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = let block_prefix_after_conversion = match block_type with | Virtio_blk -> "vd" - | Virtio_SCSI -> "sd" | IDE -> ide_block_prefix in

let map = diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml index 5c149afbb..4b56a6e17 100644 --- a/convert/target_bus_assignment.ml +++ b/convert/target_bus_assignment.ml @@ -35,7 +35,6 @@ let rec target_bus_assignment source_disks source_removables guestcaps = let bus = match guestcaps.gcaps_block_bus with | Virtio_blk -> virtio_blk_bus - | Virtio_SCSI -> scsi_bus | IDE -> ide_bus in List.iteri ( fun i d -> diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml index 470a90bd6..3991b66dd 100644 --- a/lib/create_ovf.ml +++ b/lib/create_ovf.ml @@ -891,7 +891,6 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:disk-interface", (match guestcaps.gcaps_block_bus with | Virtio_blk -> "VirtIO" - | Virtio_SCSI -> "VirtIO_SCSI" | IDE -> "IDE"); "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_bootable_drive then "True" else "False"; diff --git a/lib/types.ml b/lib/types.ml index 9be3e6fcd..aeb7d71ae 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -422,14 +422,13 @@ type guestcaps = { gcaps_arch : string; gcaps_acpi : bool; } -and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE +and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_video_type = QXL | Cirrus and guestcaps_machine = I440FX | Q35 | Virt

let string_of_block_type = function | Virtio_blk -> "virtio-blk" - | Virtio_SCSI -> "virtio-scsi" | IDE -> "ide" let string_of_net_type = function | Virtio_net -> "virtio-net" diff --git a/lib/types.mli b/lib/types.mli index 4d4049605..3aab1ea7e 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -282,7 +282,7 @@ type guestcaps = { } (** Guest capabilities after conversion. eg. Was virtio found or installed? *)

-and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE +and guestcaps_block_type = Virtio_blk | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_video_type = QXL | Cirrus and guestcaps_machine = I440FX | Q35 | Virt diff --git a/output/create_json.ml b/output/create_json.ml index 5fa2805a2..211cd5d64 100644 --- a/output/create_json.ml +++ b/output/create_json.ml @@ -195,7 +195,6 @@ let create_json_metadata source inspect let block_bus = match guestcaps.gcaps_block_bus with | Virtio_blk -> "virtio-blk" - | Virtio_SCSI -> "virtio-scsi" | IDE -> "ide" in let net_bus = match guestcaps.gcaps_net_bus with diff --git a/output/openstack_image_properties.ml b/output/openstack_image_properties.ml index 66f539921..bca0766e2 100644 --- a/output/openstack_image_properties.ml +++ b/output/openstack_image_properties.ml @@ -35,7 +35,6 @@ let create source inspect { target_buses; guestcaps; target_firmware } = "hw_disk_bus", (match guestcaps.gcaps_block_bus with | Virtio_blk -> "virtio" - | Virtio_SCSI -> "scsi" | IDE -> "ide"); "hw_vif_model", (match guestcaps.gcaps_net_bus with @@ -73,12 +72,6 @@ let create source inspect { target_buses; guestcaps; target_firmware } = List.push_back properties ("hw_cpu_threads", string_of_int threads); );

- (match guestcaps.gcaps_block_bus with - | Virtio_SCSI -> - List.push_back properties ("hw_scsi_model", "virtio-scsi") - | Virtio_blk | IDE -> () - ); - (match inspect.i_major_version, inspect.i_minor_version with | 0, 0 -> () | x, 0 -> List.push_back properties ("os_version", string_of_int x)