On Fri, Jan 28, 2022 at 05:12:13PM +0100, Laszlo Ersek wrote: > For Windows guests, the "inspect.i_arch" field is ultimately determined in > libguestfs, on the following call path: > > inspect_os [daemon/inspect.ml] > check_for_filesystem_on [daemon/inspect_fs.ml] > check_filesystem [daemon/inspect_fs.ml] > check_windows_root [daemon/inspect_fs_windows.ml] > check_windows_arch [daemon/inspect_fs_windows.ml] > file_architecture [daemon/filearch.ml] > file_architecture_of_magic [daemon/filearch.ml] > > where the last function maps "PE32 executable" to "i386". > > (As of libguestfs commit 5858c2cf6c24.) > > However, in osinfo-db (as of commit 72c69622e6db), the > "data/schema/osinfo.rng.in" schema calls the same architecture "i686". > > Perform this mapping in the "copy_from_libosinfo" function explicitly: the > filter currently throws away all "i686" drivers from libosinfo because > they don't match "i386" from libguestfs. > > (There is no such problem with "x86_64".) > > Fixes: 258e4b718d5d80c79634fe864c8c52b12e41777c > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2043333 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > convert/windows_virtio.ml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml > index d21ee6814458..1faba8a874c0 100644 > --- a/convert/windows_virtio.ml > +++ b/convert/windows_virtio.ml > @@ -412,12 +412,17 @@ and copy_from_libosinfo g inspect destdir = > List.iter ( > fun d -> > debug "\t%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". > + *) > + let arch = if arch = "i386" then "i686" else arch in Sort of the reverse of Std_utils.normalize_arch,

but I understand why it's being done here. (I wonder if we want to call it "libosinfo_arch" to avoid confusion?) Anyway, ACK

Rich. > try > let os = Libosinfo_utils.get_os_by_short_id osinfo in > let drivers = os#get_device_drivers () in > debug "libosinfo drivers before filtering:"; debug_drivers drivers; > (* > * Filter out drivers that we cannot use: > -- > 2.19.1.3.g30247aa5d201 > > > _______________________________________________ > Libguestfs mailing list > Libguestfs(a)redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs

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; --- 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); +}

On Fri, Jan 28, 2022 at 05:12:17PM +0100, Laszlo Ersek wrote: > The "copy_from_libosinfo" function in "windows_virtio.ml" filters and > sorts the driver list from libosinfo in order to find the best driver. > Move this logic to a separate function (called "best_driver") in the > Libosinfo_utils module. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2043333 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > convert/libosinfo_utils.mli | 12 ++++++++++ > convert/libosinfo_utils.ml | 35 +++++++++++++++++++++++++++++ > convert/windows_virtio.ml | 44 +------------------------------------ > 3 files changed, 48 insertions(+), 43 deletions(-) > > diff --git a/convert/libosinfo_utils.mli b/convert/libosinfo_utils.mli > index 67be16c491a4..14991bc2d782 100644 > --- a/convert/libosinfo_utils.mli > +++ b/convert/libosinfo_utils.mli > @@ -28,12 +28,24 @@ val get_os_by_short_id : string -> Libosinfo.osinfo_os > 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. *) > > +val best_driver : Libosinfo.osinfo_device_driver list -> > + string -> > + Libosinfo.osinfo_device_driver > +(** [best_driver drivers arch] picks the best driver from [drivers] as follows: > + - filters out drivers that: > + - target a different architecture, > + - are not pre-installable, > + - have an invalid or non-local URL; > + - sorts the remaining drivers by priority, like libosinfo does; > + - picks the top driver of the sorted list. > + Raises Not_found if no driver in [drivers] survives filtering. *) > + > 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 f0d70ffd4997..8504e2b2f001 100644 > --- a/convert/libosinfo_utils.ml > +++ b/convert/libosinfo_utils.ml > @@ -76,12 +76,47 @@ let string_of_osinfo_device_driver { Libosinfo.architecture; location; > (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) > > +let best_driver drivers arch = > + let debug_drivers = > + List.iter (fun d -> debug "Driver: %s" (string_of_osinfo_device_driver d)) > + (* The architecture that "inspect.i_arch" from libguestfs > + * ("daemon/filearch.ml") calls "i386", the osinfo-db schema > + * ("data/schema/osinfo.rng.in") calls "i686". > + *) > + and arch = if arch = "i386" then "i686" else arch in > + debug "libosinfo drivers before filtering:"; > + debug_drivers drivers; > + let drivers = > + List.filter ( > + fun { Libosinfo.architecture; location; pre_installable } -> > + if architecture <> arch || not pre_installable then > + false > + else > + try > + (match Xml.parse_uri location with > + | { Xml.uri_scheme = Some scheme; > + Xml.uri_path = Some _ } when scheme = "file" -> true > + | _ -> false > + ) > + with Invalid_argument _ -> false > + ) drivers in > + debug "libosinfo drivers after filtering:"; > + debug_drivers drivers; > + let drivers = > + List.sort ( > + fun { Libosinfo.priority = prioA } { Libosinfo.priority = prioB } -> > + compare prioB prioA > + ) drivers in > + if drivers = [] then > + raise Not_found; > + List.hd drivers > + > 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 610a56857c7e..5254322c7d28 100644 > --- a/convert/windows_virtio.ml > +++ b/convert/windows_virtio.ml > @@ -405,59 +405,17 @@ and virtio_iso_path_matches_qemu_ga path inspect = > * > * Files that do not exist are silently skipped. > * > * Returns list of copied files. > *) > and copy_from_libosinfo g inspect destdir = > - let debug_drivers = > - List.iter ( > - fun 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". > - *) > - let arch = if arch = "i386" then "i686" else arch in > try > let os = Libosinfo_utils.get_os_by_short_id osinfo in > let drivers = os#get_device_drivers () in > - debug "libosinfo drivers before filtering:"; debug_drivers drivers; > - (* > - * Filter out drivers that we cannot use: > - * - for a different architecture > - * - non-pre-installable ones > - * - location is an invalid URL, or a non-local one > - *) > - let drivers = > - List.filter ( > - fun { Libosinfo.architecture; location; pre_installable } -> > - if architecture <> arch || not pre_installable then > - false > - else > - try > - (match Xml.parse_uri location with > - | { Xml.uri_scheme = Some scheme; > - Xml.uri_path = Some _ } when scheme = "file" -> true > - | _ -> false > - ) > - with Invalid_argument _ -> false > - ) drivers in > - debug "libosinfo drivers after filtering:"; debug_drivers drivers; > - (* Sort the drivers by priority, like libosinfo does. *) > - let drivers = > - List.sort ( > - fun { Libosinfo.priority = prioA } { Libosinfo.priority = prioB } -> > - compare prioB prioA > - ) drivers in > - (* Any driver available? *) > - if drivers = [] then > - raise Not_found; > - let driver = List.hd drivers in > + let driver = Libosinfo_utils.best_driver drivers arch in > let uri = Xml.parse_uri driver.Libosinfo.location in > let basedir = > match uri.Xml.uri_path with > | Some p -> p > | None -> assert false in > List.filter_map ( > -- > 2.19.1.3.g30247aa5d201 > Nice refactoring, ACK