On 01/28/22 18:08, Richard W.M. Jones wrote:
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>
> ---
> 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
Thank you, I've been learning that refactorings are doable in functional
languages too!
Laszlo