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

[Richard W.M. Jones]

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