Libguestfs - Jan 2022 - [v2v PATCH 6/9] convert/libosinfo: wrap osinfo_os_get_all_devices()

Introduce the "osinfo_os.get_devices" OCaml method, for wrapping the
libosinfo API osinfo_os_get_all_devices().

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1942325
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 convert/libosinfo.mli | 14 +++++
 convert/libosinfo.ml  | 14 +++++
 convert/libosinfo-c.c | 66 ++++++++++++++++++++
 3 files changed, 94 insertions(+)

diff --git a/convert/libosinfo.mli b/convert/libosinfo.mli
index 0428ef911088..1ece7b41a310 100644
--- a/convert/libosinfo.mli
+++ b/convert/libosinfo.mli
@@ -29,11 +29,25 @@ type osinfo_device_driver = {
   files : string list;
 }
 
+type osinfo_device = {
+  id : string;
+  vendor : string;
+  vendor_id : string;
+  product : string;
+  product_id : string;
+  name : string;
+  class_ : string;
+  bus_type : string;
+  subsystem : string;
+}
+
 class osinfo_os : osinfo_os_t -> object
   method get_id : unit -> string
   (** Return the ID. *)
   method get_device_drivers : unit -> osinfo_device_driver list
   (** Return the list of device drivers. *)
+  method get_devices : unit -> osinfo_device list
+  (** Return the list of devices. *)
 end
 (** Minimal OsinfoOs wrapper. *)
 
diff --git a/convert/libosinfo.ml b/convert/libosinfo.ml
index bd9ca126f3c0..78271be248ea 100644
--- a/convert/libosinfo.ml
+++ b/convert/libosinfo.ml
@@ -32,13 +32,27 @@ type osinfo_device_driver = {
   files : string list;
 }
 
+type osinfo_device = {
+  id : string;
+  vendor : string;
+  vendor_id : string;
+  product : string;
+  product_id : string;
+  name : string;
+  class_ : string;
+  bus_type : string;
+  subsystem : string;
+}
+
 external osinfo_os_get_id : osinfo_os_t -> string =
"v2v_osinfo_os_get_id"
 external osinfo_os_get_device_drivers : osinfo_os_t -> osinfo_device_driver
list = "v2v_osinfo_os_get_device_drivers"
+external osinfo_os_get_devices : osinfo_os_t -> osinfo_device list =
"v2v_osinfo_os_get_all_devices"
 
 class osinfo_os h    object (self)
     method get_id () = osinfo_os_get_id h
     method get_device_drivers () = osinfo_os_get_device_drivers h
+    method get_devices () = osinfo_os_get_devices h
 end
 
 external osinfo_db_load : unit -> osinfo_db_t =
"v2v_osinfo_db_load"
diff --git a/convert/libosinfo-c.c b/convert/libosinfo-c.c
index 09cf588d1c10..7a8e287aeb26 100644
--- a/convert/libosinfo-c.c
+++ b/convert/libosinfo-c.c
@@ -50,6 +50,7 @@
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(OsinfoFilter, g_object_unref)
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(OsinfoLoader, g_object_unref)
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(OsinfoOsList, g_object_unref)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(OsinfoDeviceList, g_object_unref)
 #endif
 
 typedef OsinfoDb *OsinfoDb_t;
@@ -255,3 +256,68 @@ v2v_osinfo_os_get_device_drivers (value osv)
 
   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
+ *   OsinfoDevice class.
+ *
+ * All of the above properties have string values. Thus, for uniformity, access
+ * all these properties by their names at the OsinfoEntity level (i.e., forego
+ * the class- and property-specific, dedicated property getter functions).
+ */
+static const char * const device_prop[] = {
+  OSINFO_ENTITY_PROP_ID,
+  OSINFO_DEVICE_PROP_VENDOR,
+  OSINFO_DEVICE_PROP_VENDOR_ID,
+  OSINFO_DEVICE_PROP_PRODUCT,
+  OSINFO_DEVICE_PROP_PRODUCT_ID,
+  OSINFO_DEVICE_PROP_NAME,
+  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)
+{
+  CAMLparam1 (osv);
+  CAMLlocal3 (retval, link, props);
+  g_autoptr (OsinfoDeviceList) dev_list = NULL;
+  OsinfoList *ent_list;
+  gint ent_nr;
+
+  retval = 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;
+
+    --ent_nr;
+    ent = osinfo_list_get_nth (ent_list, ent_nr);
+
+    props = caml_alloc (NUM_DEVICE_PROPS, 0);
+    for (prop_nr = 0; prop_nr < NUM_DEVICE_PROPS; ++prop_nr) {
+      const gchar *prop_val;
+
+      prop_val = osinfo_entity_get_param_value (ent, device_prop[prop_nr]);
+      if (prop_val == NULL)
+        prop_val = "";
+      Store_field (props, prop_nr, caml_copy_string (prop_val));
+    }
+
+    link = caml_alloc (2, 0);
+    Store_field (link, 0, props);
+    Store_field (link, 1, retval);
+    retval = link;
+  }
+
+  CAMLreturn (retval);
+}
-- 
2.19.1.3.g30247aa5d201

On Thu, Jan 06, 2022 at 03:09:07PM +0100, Laszlo Ersek
wrote:
> +value
> +v2v_osinfo_os_get_all_devices (value osv)
> +{
> +  CAMLparam1 (osv);
> +  CAMLlocal3 (retval, link, props);
It's a matter of style but I would tend to name variables which are
OCaml values (C type: value) as "<something>v", just to remind
myself
that they have to follow the rules; see for example:

https://github.com/libguestfs/virt-v2v/blob/f9d5448d2efec106ab452e7b219ca179752983c3/convert/libosinfo-c.c#L108
> +  g_autoptr (OsinfoDeviceList) dev_list = NULL;
> +  OsinfoList *ent_list;
> +  gint ent_nr;
> +
> +  retval = 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;
> +
> +    --ent_nr;
> +    ent = osinfo_list_get_nth (ent_list, ent_nr);
> +
> +    props = caml_alloc (NUM_DEVICE_PROPS, 0);
> +    for (prop_nr = 0; prop_nr < NUM_DEVICE_PROPS; ++prop_nr) {
> +      const gchar *prop_val;
> +
> +      prop_val = osinfo_entity_get_param_value (ent,
device_prop[prop_nr]);
> +      if (prop_val == NULL)
> +        prop_val = "";
> +      Store_field (props, prop_nr, caml_copy_string (prop_val));
This works because the struct only contains strings:

type osinfo_device = {
  id : string;
  vendor : string;
  vendor_id : string;
  product : string;
  product_id : string;
  name : string;
  class_ : string;
  bus_type : string;
  subsystem : string;
}

which is OK, but doesn't libosinfo let us convert them to more natural
types?

Another thing which is not actually a bug but is potentially
dangerous is:

  Store_field (props, prop_nr, caml_copy_string (prop_val));
                               ^^^^

A temporary variable is allocated to store the result of
caml_copy_string, so it's equivalent to:

  value tv = caml_copy_string (prop_val);
  Store_field (props, prop_nr, tv);

Now it turns out that Store_field (which calls caml_modify) is
documented to never call the garbage collector, so the block pointed
to by "tv" could never move, so we're OK!

But the code is nevertheless a bit tricky and you might want to make
the temporary explicit (and include it in the CAMLlocal expression at
the top of the function so it is a global root).
> +    }
> +
> +    link = caml_alloc (2, 0);
> +    Store_field (link, 0, props);
> +    Store_field (link, 1, retval);
> +    retval = link;
> +  }
> +
> +  CAMLreturn (retval);
So the code is fine as it turns out, but:

(a) Might consider using <something>v for value names

(b) It'd be nice if libosinfo gave us real types

(c) Better IMHO to make the temporary variable explicit

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org