4:10 a.m.
On 01/06/22 15:58, Richard W.M. Jones wrote:
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...
So that's the explanation behind these "v" suffixes. :)
OK, I can do that, but I still very much want the *prefixes* to make sense. What about:
- retval_v, link_v, props_v;
- or: retvalv, linkv, propsv?
I obviously based my function on v2v_osinfo_os_get_device_drivers(), and this line:
CAMLlocal3 (rv, v, vi);
could not have been more obscure -- it almost felt like intentional obfuscation :)
> + 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,
Yes, the code intentionally relies on this. See the comment on "device_prop":
+ * 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).
but doesn't libosinfo let us convert them to more natural
types?
Wait, I understand your question better now. Here's the thing: the *natural type* for
each of these properties is *genuinely* string. It's not like I'm accessing things
that are actually integers and booleans through their internal (low-level) string
representation -- they are *genuinely* strings.
I defined the "osinfo_device" OCaml type with purely string fields *because* all
of the OsInfoDevice properties are strings in the first place!
I wish I could provide you a URL with http(s) scheme; however, the only place where I
managed to find libosinfo documentation is my hard disk, from the
"libosinfo-devel" package. So please open this URL;
file:///usr/share/gtk-doc/html/Libosinfo/OsinfoDevice.html
and you'll see that all the properties are strings.
In fact, the first version of my array here did not contain strings (property names), but
pointers to the following getter functions (which is "all of the existent
getters"):
osinfo_device_get_vendor ()
osinfo_device_get_vendor_id ()
osinfo_device_get_product ()
osinfo_device_get_product_id ()
osinfo_device_get_bus_type ()
osinfo_device_get_class ()
osinfo_device_get_name ()
osinfo_device_get_subsystem ()
This was possible because all of them return "const gchar *".
My first version worked fine, but then I noticed that I missed the most important
property: the ID.
That property is defined by the abstract base class OsinfoEntity however:
const gchar * osinfo_entity_get_id ()
file:///usr/share/gtk-doc/html/Libosinfo/OsinfoEntity.html
So that was a complication: I would have to add some kind of dynamic casting, because
osinfo_entity_get_id () would act at a different level than the other getters.
Here's the simplification: if I work solely at the OsinfoEntity level, and use
osinfo_entity_get_param_value() exclusively, I can access all the properties uniformly, by
name, regardless of whether they come from the base class or from the derived class. The
property names themselves are public API; there are no tricks being pulled.
And given the actual properties at hand, the string representation returned by
osinfo_entity_get_id() matches the *actual* property types -- because all those types are
genuinely strings.
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).
The
Store_field (..., caml_copy_string (prop_val))
pattern was copied verbatim from the existent function
v2v_osinfo_os_get_device_drivers():
str = osinfo_device_driver_get_architecture (driver);
Store_field (vi, 0, caml_copy_string (str));
str = osinfo_device_driver_get_location (driver);
Store_field (vi, 1, caml_copy_string (str));
In fact, when writing my function, I meticulously reviewed the transfer (ownership)
annotations of all the other (prior) libosinfo APIs used in this C source file.
Thankfully, I found no bugs. I initially suspected that we had a leak here, in
v2v_osinfo_os_get_device_drivers():
OsinfoDeviceDriverList *list;
[...]
list = osinfo_os_get_device_drivers (OsinfoOs_t_val (osv));
I thought that it should have been wrapped into g_autoptr().
However, surprisingly, osinfo_os_get_device_drivers() is annotated with "transfer
none" (so the code is correct!):
file:///usr/share/gtk-doc/html/Libosinfo/OsinfoOs.html#osinfo-os-get-device-drivers
while the function I needed myself is annotated with "transfer full":
file:///usr/share/gtk-doc/html/Libosinfo/OsinfoOs.html#osinfo-os-get-all-devices
> + }
> +
> + 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
Sure, I'll do that (please advise me on the "v" vs. "_v" suffix)
(b) It'd be nice if libosinfo gave us real types
It does -- but the string representations I use here already matches the actual types in
question! (That's why I went with this loop-based approach in the first place, rather
than the explicit property-wise copying seen in v2v_osinfo_os_get_device_drivers().)
(c) Better IMHO to make the temporary variable explicit
Hmmm, I'm not attracted to this; it will create a discrepancy between
v2v_osinfo_os_get_device_drivers() and the new function. I took
Store_field (..., caml_copy_string (prop_val))
as a known-safe, canned pattern.
The pattern has three *prior* uses in this source file, plus we have one
CAMLreturn (caml_copy_string (id));
as well.
If the pattern is obscure (or risky), we should eliminate all uses of it.
Please comment :)
Thanks!
Laszlo