Re: [Libguestfs] [v2v PATCH 6/9] convert/libosinfo: wrap osinfo_os_get_all_devices()
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/f9d5448d2efec106ab452e7b219ca...
+ 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