[Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

[Richard W.M. Jones]

On Mon, Jan 10, 2022 at 02:26:23PM +0100, Laszlo Ersek wrote:
> Store_field() is apparently documented to never invalidate its last
> argument by calling the garbage collector, so the code we currently have
> in "libosinfo-c.c" is safe.

Or was!  Store_field is a trivial macro around caml_modify, and
caml_modify _was_ documented as not calling the GC here:

https://github.com/ocaml/ocaml/blob/b6163a49ee67acf17235b49c177a5f85b8c944db/runtime/memory.c#L609

But that comment is not present on the trunk.  (I'm unclear when it
was removed - the git history is complicated to follow.)

> However, the following warning
> <https://ocaml.org/manual/intfc.html#ss:c-simple-gc-harmony> does apply to
> Store_field():
> 
> > The first argument of Store_field and Store_double_field must be a
> > variable declared by CAMLparam* or a parameter declared by CAMLlocal* to
> > ensure that a garbage collection triggered by the evaluation of the
> > other arguments will not invalidate the first argument after it is
> > computed.
> 
> meaning that the *first* argument of Store_field() *could* be released or
> moved by a garbage collection that is triggered by the evaluation of the
> last argument. And, in order to protect the first arg from such a release
> (or, to be notified if the first arg is moved), the first arg must be
> (temporarily) registered as a "root" value (with the "rootness"
> substituting for other inward edge(s) from other blocks).
> 
> Keeping in mind the first and last (block) args' *different* exposures to
> garbage collection is difficult. Therefore, for every Store_field() call
> where the last arg is not an unboxed integer (in practice: Val_bool()),
> but a block created with caml_copy_*(), track that block (at least
> temporarily -- "long enough") as a root value, via a new local CAML value
> called "copyv".
> 
> For consistency, update the one
> 
>   CAMLreturn (caml_copy_string (id))
> 
> call we have as well.
> 
> Suggested-by: Richard W.M. Jones <rjones at redhat.com>
> Ref: https://listman.redhat.com/archives/libguestfs/2022-January/msg00037.html
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  convert/libosinfo-c.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/convert/libosinfo-c.c b/convert/libosinfo-c.c
> index b8e78bec5c1c..ec7c06d379e3 100644
> --- a/convert/libosinfo-c.c
> +++ b/convert/libosinfo-c.c
> @@ -183,23 +183,26 @@ value
>  v2v_osinfo_os_get_id (value osv)
>  {
>    CAMLparam1 (osv);
> +  CAMLlocal1 (copyv);
>    const gchar *id;
>  
>    id = osinfo_entity_get_id (OSINFO_ENTITY(OsinfoOs_t_val (osv)));
> -  CAMLreturn (caml_copy_string (id));
> +  copyv = caml_copy_string (id);
> +  CAMLreturn (copyv);
>  }
>  
>  static value
>  glist_to_value_list (GList *list)
>  {
>    CAMLparam0 ();
> -  CAMLlocal2 (rv, v);
> +  CAMLlocal3 (rv, v, copyv);
>    GList *l;
>  
>    rv = Val_emptylist;
>    for (l = list; l != NULL; l = l->next) {
>      v = caml_alloc (2, 0);
> -    Store_field (v, 0, caml_copy_string (l->data));
> +    copyv = caml_copy_string (l->data);
> +    Store_field (v, 0, copyv);
>      Store_field (v, 1, rv);
>      rv = v;
>    }
> @@ -211,7 +214,7 @@ value
>  v2v_osinfo_os_get_device_drivers (value osv)
>  {
>    CAMLparam1 (osv);
> -  CAMLlocal3 (rv, v, vi);
> +  CAMLlocal4 (rv, v, vi, copyv);
>    OsinfoDeviceDriverList *list;
>    gint i, len;
>  
> @@ -230,9 +233,11 @@ v2v_osinfo_os_get_device_drivers (value osv)
>  
>      vi = caml_alloc (6, 0);
>      str = osinfo_device_driver_get_architecture (driver);
> -    Store_field (vi, 0, caml_copy_string (str));
> +    copyv = caml_copy_string (str);
> +    Store_field (vi, 0, copyv);
>      str = osinfo_device_driver_get_location (driver);
> -    Store_field (vi, 1, caml_copy_string (str));
> +    copyv = caml_copy_string (str);
> +    Store_field (vi, 1, copyv);
>      b = osinfo_device_driver_get_pre_installable (driver);
>      Store_field (vi, 2, Val_bool (b));
>      b = osinfo_device_driver_get_signed (driver);
> @@ -243,7 +248,8 @@ v2v_osinfo_os_get_device_drivers (value osv)
>      /* Same as OSINFO_DEVICE_DRIVER_DEFAULT_PRIORITY in libosinfo 1.7.0+. */
>      i64 = 50;
>  #endif
> -    Store_field (vi, 4, caml_copy_int64 (i64));
> +    copyv = caml_copy_int64 (i64);
> +    Store_field (vi, 4, copyv);
>      l = osinfo_device_driver_get_files (driver);
>      Store_field (vi, 5, glist_to_value_list (l));
>      g_list_free (l);
> @@ -286,7 +292,7 @@ value
>  v2v_osinfo_os_get_all_devices (value osv)
>  {
>    CAMLparam1 (osv);
> -  CAMLlocal3 (retvalv, linkv, propsv);
> +  CAMLlocal4 (retvalv, linkv, propsv, copyv);
>    g_autoptr (OsinfoDeviceList) dev_list = NULL;
>    OsinfoList *ent_list;
>    gint ent_nr;
> @@ -310,7 +316,8 @@ v2v_osinfo_os_get_all_devices (value osv)
>        prop_val = osinfo_entity_get_param_value (ent, device_prop[prop_nr]);
>        if (prop_val == NULL)
>          prop_val = "";
> -      Store_field (propsv, prop_nr, caml_copy_string (prop_val));
> +      copyv = caml_copy_string (prop_val);
> +      Store_field (propsv, prop_nr, copyv);
>      }
>  
>      linkv = caml_alloc (2, 0);
> 
> base-commit: f0cea012d0183edf6f7b769c28d5038593f3fe6a

ACK

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html