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/f9d5448d2efec106ab452e7b219ca179752983c3/convert/libosinfo-c.c#L108
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
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
