Commit 258e4b718d5d introduced the "copy_from_libosinfo" branch in "copy_drivers" with higher priority than the existent "copy_from_virtio_win". This introduced a conflict with the documentation ("docs/virt-v2v.pod"). The documentation stated (and still states) that the VIRTIO_WIN env var, when set, dictates where virt-v2v look for the virtio-win drivers. The conflict is that, even in case we set VIRTIO_WIN, virt-v2v still consults libosinfo first. Skip the "copy_from_libosinfo" branch in case VIRTIO_WIN is set, plus explain in the documentation that VIRTIO_WIN, when set, overrides libosinfo. NOTE: the necessity of this fix is made apparent by the next patch in the series. Namely, "copy_from_libosinfo" has a bug in the architecture filter. Once we fix that bug in the next patch, *and* install (e.g.) "virtio-win-1.9.24-4.el8.noarch" on the conversion host, the "i-ova" test case will suddenly prefer the virtio drivers exposed via libosinfo, in spite of the "tests/test-v2v-i-ova.sh" script explicitly setting VIRTIO_WIN to "test-data/fake-virtio-win". The symptom is that the pvpanic device will be enabled in the domain XML, due to it being supported by the actual (not fake) virtio-win drivers. Fixes: 258e4b718d5d80c79634fe864c8c52b12e41777c Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2043333 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: v2: - no change convert/windows_virtio.ml | 12 ++++++------ docs/virt-v2v.pod | 24 +++++++++++++++++------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml index 1c5c148e65fd..d21ee6814458 100644 --- a/convert/windows_virtio.ml +++ b/convert/windows_virtio.ml @@ -26,20 +26,20 @@ open Regedit open Types open Utils module G = Guestfs -let virtio_win = - try Sys.getenv "VIRTIO_WIN" +let virtio_win, virtio_win_from_env = + try Sys.getenv "VIRTIO_WIN", true with Not_found -> - try Sys.getenv "VIRTIO_WIN_DIR" (* old name for VIRTIO_WIN *) + try Sys.getenv "VIRTIO_WIN_DIR" (* old name for VIRTIO_WIN *), true with Not_found -> let iso = Config.datadir // "virtio-win" // "virtio-win.iso" in - if Sys.file_exists iso then iso - else Config.datadir // "virtio-win" + (if Sys.file_exists iso then iso + else Config.datadir // "virtio-win"), false let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}" let viostor_legacy_pciid = "VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00" 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" @@ -225,13 +225,13 @@ and ddb_regedits inspect drv_name drv_pciid = ] (* Copy the matching drivers to the driverdir; return true if any have * been copied. *) and copy_drivers g inspect driverdir = - [] <> copy_from_libosinfo g inspect driverdir || + (not virtio_win_from_env && [] <> copy_from_libosinfo g inspect driverdir) || [] <> copy_from_virtio_win g inspect "/" driverdir virtio_iso_path_matches_guest_os (fun () -> error (f_"root directory ‘/’ is missing from the virtio-win directory or ISO.\n\nThis should not happen and may indicate that virtio-win or virt-v2v is broken in some way. Please report this as a bug with a full debug log.")) and copy_qemu_ga g inspect = diff --git a/docs/virt-v2v.pod b/docs/virt-v2v.pod index 3d0e00a354ae..89c47cd3823c 100644 --- a/docs/virt-v2v.pod +++ b/docs/virt-v2v.pod @@ -916,13 +916,18 @@ below. Debian 6+ All versions support virtio Ubuntu 10.04+ All versions support virtio Windows Drivers are installed from the ISO or directory pointed - to by "VIRTIO_WIN" environment variable if present + to by the "VIRTIO_WIN" environment variable if present. + If the "VIRTIO_WIN" environment variable is absent + (which is the recommended setting), then libosinfo is + consulted first, for driver files that are locally + available on the conversion host. + =head2 RHEL 4: SELinux relabel appears to hang forever In RHEL E<le> 4.7 there was a bug which causes SELinux relabelling to appear to hang forever at: @@ -1592,29 +1597,34 @@ script in Windows guests. It is required if you intend to use the I<--firstboot> or I<--firstboot-command> options with Windows guests. =back =item C<VIRTIO_WIN> -This is where virtio drivers for Windows are searched for. It can be -a directory I<or> point to F<virtio-win.iso> (CD ROM image containing -drivers). +This is an override for where virtio drivers for Windows are searched +for. It can be a directory I<or> point to F<virtio-win.iso> (CD ROM +image containing drivers). -If unset, then we look for drivers in whichever of these paths -is found first: +If unset, then we look for drivers via whichever of these methods +succeeds first: =over 4 +=item C<osinfo-db> + +Load osinfo data from the default paths, and attempt to find drivers via +libosinfo lookup. This is the preferred method. + =item F</usr/share/virtio-win/virtio-win.iso> The ISO containing virtio drivers for Windows. =item F</usr/share/virtio-win> The exploded tree of virtio drivers for Windows. This is -usually incomplete, hence the ISO is preferred. +usually incomplete, hence the least preferred method. =back See L</Enabling virtio>. =back -- 2.19.1.3.g30247aa5d201

Move the guts of v2v_osinfo_os_get_all_devices() to a new static function called v2v_osinfo_device_list_to_value_list(). Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2043333 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Acked-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v2: - pick up Rich's ACK convert/libosinfo-c.c | 126 +++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 58 deletions(-) diff --git a/convert/libosinfo-c.c b/convert/libosinfo-c.c index ec7c06d379e3..bc5816f6dacb 100644 --- a/convert/libosinfo-c.c +++ b/convert/libosinfo-c.c @@ -207,65 +207,12 @@ glist_to_value_list (GList *list) rv = v; } CAMLreturn (rv); } -value -v2v_osinfo_os_get_device_drivers (value osv) -{ - CAMLparam1 (osv); - CAMLlocal4 (rv, v, vi, copyv); - OsinfoDeviceDriverList *list; - gint i, len; - - list = osinfo_os_get_device_drivers (OsinfoOs_t_val (osv)); - len = osinfo_list_get_length (OSINFO_LIST(list)); - - rv = Val_emptylist; - for (i = len - 1; i >= 0; --i) { - OsinfoDeviceDriver *driver; - const gchar *str; - gboolean b; - GList *l; - gint64 i64; - - driver = OSINFO_DEVICE_DRIVER(osinfo_list_get_nth (OSINFO_LIST(list), i)); - - vi = caml_alloc (6, 0); - str = osinfo_device_driver_get_architecture (driver); - copyv = caml_copy_string (str); - Store_field (vi, 0, copyv); - str = osinfo_device_driver_get_location (driver); - copyv = caml_copy_string (str); - Store_field (vi, 1, copyv); - b = osinfo_device_driver_get_pre_installable (driver); - Store_field (vi, 2, Val_bool (b)); - b = osinfo_device_driver_get_signed (driver); - Store_field (vi, 3, Val_bool (b)); -#if IS_LIBOSINFO_VERSION(1, 7, 0) - i64 = osinfo_device_driver_get_priority (driver); -#else - /* Same as OSINFO_DEVICE_DRIVER_DEFAULT_PRIORITY in libosinfo 1.7.0+. */ - i64 = 50; -#endif - copyv = caml_copy_int64 (i64); - Store_field (vi, 4, copyv); - l = osinfo_device_driver_get_files (driver); - Store_field (vi, 5, glist_to_value_list (l)); - g_list_free (l); - - v = caml_alloc (2, 0); - Store_field (v, 0, vi); - Store_field (v, 1, rv); - rv = v; - } - - CAMLreturn (rv); -} - /* Collect OsinfoDevice properties from two levels: * * - The OSINFO_ENTITY_PROP_ID property, originating from the OsinfoEntity base * class. This is a unique URI, identifying the device. * * - All currently known OSINFO_DEVICE_PROP_* properties, originating from the @@ -285,23 +232,21 @@ static const char * const device_prop[] = { OSINFO_DEVICE_PROP_CLASS, OSINFO_DEVICE_PROP_BUS_TYPE, OSINFO_DEVICE_PROP_SUBSYSTEM, }; #define NUM_DEVICE_PROPS (sizeof device_prop / sizeof device_prop[0]) -value -v2v_osinfo_os_get_all_devices (value osv) +static value +v2v_osinfo_device_list_to_value_list (OsinfoDeviceList *dev_list) { - CAMLparam1 (osv); + CAMLparam0 (); CAMLlocal4 (retvalv, linkv, propsv, copyv); - g_autoptr (OsinfoDeviceList) dev_list = NULL; OsinfoList *ent_list; gint ent_nr; retvalv = Val_emptylist; - dev_list = osinfo_os_get_all_devices (OsinfoOs_t_val (osv), NULL); ent_list = OSINFO_LIST (dev_list); ent_nr = osinfo_list_get_length (ent_list); while (ent_nr > 0) { OsinfoEntity *ent; size_t prop_nr; @@ -325,6 +270,71 @@ v2v_osinfo_os_get_all_devices (value osv) Store_field (linkv, 1, retvalv); retvalv = linkv; } CAMLreturn (retvalv); } + +value +v2v_osinfo_os_get_device_drivers (value osv) +{ + CAMLparam1 (osv); + CAMLlocal4 (rv, v, vi, copyv); + OsinfoDeviceDriverList *list; + gint i, len; + + list = osinfo_os_get_device_drivers (OsinfoOs_t_val (osv)); + len = osinfo_list_get_length (OSINFO_LIST(list)); + + rv = Val_emptylist; + for (i = len - 1; i >= 0; --i) { + OsinfoDeviceDriver *driver; + const gchar *str; + gboolean b; + GList *l; + gint64 i64; + + driver = OSINFO_DEVICE_DRIVER(osinfo_list_get_nth (OSINFO_LIST(list), i)); + + vi = caml_alloc (6, 0); + str = osinfo_device_driver_get_architecture (driver); + copyv = caml_copy_string (str); + Store_field (vi, 0, copyv); + str = osinfo_device_driver_get_location (driver); + copyv = caml_copy_string (str); + Store_field (vi, 1, copyv); + b = osinfo_device_driver_get_pre_installable (driver); + Store_field (vi, 2, Val_bool (b)); + b = osinfo_device_driver_get_signed (driver); + Store_field (vi, 3, Val_bool (b)); +#if IS_LIBOSINFO_VERSION(1, 7, 0) + i64 = osinfo_device_driver_get_priority (driver); +#else + /* Same as OSINFO_DEVICE_DRIVER_DEFAULT_PRIORITY in libosinfo 1.7.0+. */ + i64 = 50; +#endif + copyv = caml_copy_int64 (i64); + Store_field (vi, 4, copyv); + l = osinfo_device_driver_get_files (driver); + Store_field (vi, 5, glist_to_value_list (l)); + g_list_free (l); + + v = caml_alloc (2, 0); + Store_field (v, 0, vi); + Store_field (v, 1, rv); + rv = v; + } + + CAMLreturn (rv); +} + +value +v2v_osinfo_os_get_all_devices (value osv) +{ + CAMLparam1 (osv); + CAMLlocal1 (retvalv); + g_autoptr (OsinfoDeviceList) dev_list = NULL; + + dev_list = osinfo_os_get_all_devices (OsinfoOs_t_val (osv), NULL); + retvalv = v2v_osinfo_device_list_to_value_list (dev_list); + CAMLreturn (retvalv); +} -- 2.19.1.3.g30247aa5d201

While at it, put each file of a driver "pack" on a separate line, and print "Driver:", "Files:" and "Devices:" headers as well. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2043333 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Acked-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v2: - pick up Rich's ACK convert/libosinfo_utils.mli | 6 +++--- convert/libosinfo_utils.ml | 21 +++++++++++---------- convert/windows_virtio.ml | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/convert/libosinfo_utils.mli b/convert/libosinfo_utils.mli index ab77ec97df30..67be16c491a4 100644 --- a/convert/libosinfo_utils.mli +++ b/convert/libosinfo_utils.mli @@ -22,18 +22,18 @@ val get_os_by_short_id : string -> Libosinfo.osinfo_os (** [get_os_by_short_id short-id] get the [Libosinfo.osinfo_os] that has the specified [short-id]. Raise [Not_found] in case there is no matching OS. *) -val string_of_osinfo_device_driver : Libosinfo.osinfo_device_driver -> string -(** Convert a [osinfo_device_driver] to a printable string for debugging. *) - val string_of_osinfo_device_list : Libosinfo.osinfo_device list -> string (** Convert an [osinfo_device] list to a printable string for debugging. *) +val string_of_osinfo_device_driver : Libosinfo.osinfo_device_driver -> string +(** Convert a [osinfo_device_driver] to a printable string for debugging. *) + type os_support = { q35 : bool; vio10 : bool; } (** Tell whether the operating system supports the Q35 board type and/or non-transitional (virtio-1.0-only) virtio devices. (Internally, the diff --git a/convert/libosinfo_utils.ml b/convert/libosinfo_utils.ml index 77f22272f979..f0d70ffd4997 100644 --- a/convert/libosinfo_utils.ml +++ b/convert/libosinfo_utils.ml @@ -30,22 +30,12 @@ let get_db () = let get_os_by_short_id os = let os = (get_db ())#find_os_by_short_id os in debug "libosinfo: loaded OS: %s" (os#get_id ()); os -let string_of_osinfo_device_driver { Libosinfo.architecture; location; - pre_installable; signed; priority; - files } = - Printf.sprintf "%s: [%s, %s, %s, priority %Ld] %s" - location architecture - (if pre_installable then "pre-installable" else "not pre-installable") - (if signed then "signed" else "unsigned") - priority - (String.concat " " files) - let string_of_osinfo_device_list dev_list = (* Turn the fields of an "osinfo_device" record into a list. *) let listify { Libosinfo.id; vendor; vendor_id; product; product_id; name; class_; bus_type; subsystem } = [ id; vendor; vendor_id; product; product_id; name; @@ -75,12 +65,23 @@ let string_of_osinfo_device_list dev_list = * per line, and (b) right-padding each field of each "osinfo_device" record * to the maximum width of that field. *) String.concat "\n" (List.map (fun dev -> columnate (listify dev) max_widths) dev_list) +let string_of_osinfo_device_driver { Libosinfo.architecture; location; + pre_installable; signed; priority; + files; devices } = + Printf.sprintf "%s: [%s, %s, %s, priority %Ld]\nFiles:\n%s\nDevices:\n%s" + location architecture + (if pre_installable then "pre-installable" else "not pre-installable") + (if signed then "signed" else "unsigned") + priority + (String.concat "\n" files) + (string_of_osinfo_device_list devices) + type os_support = { q35 : bool; vio10 : bool; } let os_support_of_osinfo_device_list = diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml index 1faba8a874c0..610a56857c7e 100644 --- a/convert/windows_virtio.ml +++ b/convert/windows_virtio.ml @@ -408,13 +408,13 @@ and virtio_iso_path_matches_qemu_ga path inspect = * Returns list of copied files. *) and copy_from_libosinfo g inspect destdir = let debug_drivers = List.iter ( fun d -> - debug "\t%s" (Libosinfo_utils.string_of_osinfo_device_driver d) + debug "Driver: %s" (Libosinfo_utils.string_of_osinfo_device_driver d) ) in let { i_osinfo = osinfo; i_arch = arch } = inspect in (* The architecture that "inspect.i_arch" from libguestfs * ("daemon/filearch.ml") calls "i386", the osinfo-db schema * ("data/schema/osinfo.rng.in") calls "i686". -- 2.19.1.3.g30247aa5d201

Life would be too simple if we could just call osinfo_os_get_all_devices() for determining virtio-1.0 support for Windows guests. Now that we've extracted the libosinfo logic that identifies the "best" virtio driver (for copying its files into the guest), fetch the list of devices supported by the same "best driver" as well. Use that list, in addition to the one from osinfo_os_get_all_devices(), for determining (Q35 and) virtio-1.0 support. Thankfully this mess is not needed when converting Linux guests. With this, we need to revert the data for three Windows-based test cases to their pre-f0cea012d018 status (modulo intermediary commits 4f6b143c1cb3 ("output: -o libvirt, qemu: Use correct device name for vsock", 2022-01-20) and 4c3d0b8b3b4b ("output: -o libvirt: Fix <graphics/> element port/autoport", 2022-01-20)). The reason is that those tests all set the VIRTIO_WIN environment variable, in which case we simply assume "virtio_1_0 = true" on the fallback ("Not_found" exception) branch. Fixes: f0cea012d0183edf6f7b769c28d5038593f3fe6a Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2043333 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: v2: - mirror the behavior of "Windows_virtio.copy_drivers" exactly, with regard to: - the VIRTIO_WIN(_DIR) environment variables, and - "Libosinfo_utils.best_driver" not finding any matching driver. (The behavior upon "Libosinfo_utils.get_os_by_short_id" failing was consistent with "Windows_virtio.copy_drivers" in v1 already.) convert/windows_virtio.mli | 6 +++++- convert/convert_windows.ml | 26 +++++++++++++++++++++++--- tests/test-v2v-cdrom.expected | 2 +- tests/test-v2v-floppy.expected | 2 +- tests/test-v2v-i-ova.xml | 8 ++++---- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/convert/windows_virtio.mli b/convert/windows_virtio.mli index 53603d240553..a92cc01da6df 100644 --- a/convert/windows_virtio.mli +++ b/convert/windows_virtio.mli @@ -13,13 +13,17 @@ * * You should have received a copy of the GNU General Public License along * with this program; if not, write to the Free Software Foundation, Inc., * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. *) -(** Functions for installing Windows virtio drivers. *) +(** Values and functions for installing Windows virtio drivers. *) + +val virtio_win_from_env : bool +(** [virtio_win_from_env] is true iff at least one of the VIRTIO_WIN and + VIRTIO_WIN_DIR variables is present in the environment. *) val install_drivers : Registry.t -> Types.inspect -> Types.guestcaps_block_type * Types.guestcaps_net_type * bool * bool * bool * bool (** [install_drivers reg inspect] installs virtio drivers from the driver directory or driver diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 1c2d17f26543..7bdcaf52f33c 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -239,18 +239,38 @@ let convert (g : G.guestfs) _ inspect _ static_ips = ); let machine, virtio_1_0 = match inspect.i_arch with | ("i386"|"x86_64") -> (try + (* Fall back to the decision that's based on the year that the OS + * was released in under three circumstances: + * - the user specified the location of the Windows virtio drivers + * through an environment variable, or + * - "Libosinfo_utils.get_os_by_short_id" fails to look up the OS, + * or + * - "Libosinfo_utils.best_driver" cannot find any matching driver. + * In each of these cases, a "Not_found" exception is raised. This + * behavior exactly mirrors that of "Windows_virtio.copy_drivers". + *) + if Windows_virtio.virtio_win_from_env then + raise Not_found; let os = Libosinfo_utils.get_os_by_short_id inspect.i_osinfo in - let devices = os#get_devices () in - debug "libosinfo devices for OS \"%s\":\n%s" inspect.i_osinfo + let devices = os#get_devices () + and drivers = os#get_device_drivers () in + let best_drv_devs = + (Libosinfo_utils.best_driver drivers inspect.i_arch).devices in + debug "libosinfo internal devices for OS \"%s\":\n%s" + inspect.i_osinfo (Libosinfo_utils.string_of_osinfo_device_list devices); + debug "libosinfo \"best driver\" devices for OS \"%s\":\n%s" + inspect.i_osinfo + (Libosinfo_utils.string_of_osinfo_device_list best_drv_devs); let { Libosinfo_utils.q35; vio10 } = - Libosinfo_utils.os_support_of_osinfo_device_list devices in + Libosinfo_utils.os_support_of_osinfo_device_list + (devices @ best_drv_devs) in (if q35 then Q35 else I440FX), vio10 with | Not_found -> (* Pivot on the year 2007. Any Windows version from earlier than * 2007 should use i440fx, anything 2007 or newer should use q35. * Luckily this coincides almost exactly with the release of NT 6. diff --git a/tests/test-v2v-cdrom.expected b/tests/test-v2v-cdrom.expected index b950492969c8..17bd152d8e64 100644 --- a/tests/test-v2v-cdrom.expected +++ b/tests/test-v2v-cdrom.expected @@ -1,7 +1,7 @@ - <disk type='file' device='disk' model='virtio-transitional'> + <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <target dev='vda' bus='virtio'/> </disk> <disk device='cdrom' type='file'> <driver name='qemu' type='raw'/> <target dev='sdc' bus='sata'/> diff --git a/tests/test-v2v-floppy.expected b/tests/test-v2v-floppy.expected index f4b679540c9d..a718c21f94dd 100644 --- a/tests/test-v2v-floppy.expected +++ b/tests/test-v2v-floppy.expected @@ -1,7 +1,7 @@ - <disk type='file' device='disk' model='virtio-transitional'> + <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <target dev='vda' bus='virtio'/> </disk> <disk device='floppy' type='file'> <driver name='qemu' type='raw'/> <target dev='fda'/> diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml index 1915dd40aff0..6b8cda62f051 100644 --- a/tests/test-v2v-i-ova.xml +++ b/tests/test-v2v-i-ova.xml @@ -18,13 +18,13 @@ <type arch='x86_64' machine='q35'>hvm</type> </os> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>restart</on_crash> <devices> - <disk type='file' device='disk' model='virtio-transitional'> + <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='TestOva-sda'/> <target dev='vda' bus='virtio'/> </disk> <disk device='cdrom' type='file'> <driver name='qemu' type='raw'/> @@ -33,21 +33,21 @@ <disk device='floppy' type='file'> <driver name='qemu' type='raw'/> <target dev='fda'/> </disk> <interface type='bridge'> <source bridge='VM Network'/> - <model type='virtio-transitional'/> + <model type='virtio'/> </interface> <video> <model type='vga' vram='16384' heads='1'/> </video> <graphics type='vnc' autoport='yes'/> - <rng model='virtio-transitional'> + <rng model='virtio'> <backend model='random'>/dev/urandom</backend> </rng> - <memballoon model='virtio-transitional'/> + <memballoon model='virtio'/> <input type='tablet' bus='usb'/> <input type='mouse' bus='ps2'/> <console type='pty'/> </devices> </domain> -- 2.19.1.3.g30247aa5d201