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 <[email protected]> > > 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 _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
