From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Scott Subject: Re: [PATCH 08/28] libxc: ocaml: add simple binding for xentoollog (output only). Date: Tue, 26 Mar 2013 11:14:18 +0000 Message-ID: <5151830A.1090200@eu.citrix.com> References: <1364222729-6982-1-git-send-email-rob.hoes@citrix.com> <1364222729-6982-9-git-send-email-rob.hoes@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1364222729-6982-9-git-send-email-rob.hoes@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Rob Hoes Cc: Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 25/03/13 14:45, Rob Hoes wrote: [snip] > +/* external _create_logger: (string * string) -> handle = "stub_xtl_create_logger" */ > +CAMLprim value stub_xtl_create_logger(value cbs) > +{ > + CAMLparam1(cbs); > + struct caml_xtl *xtl = malloc(sizeof(*xtl)); > + if (xtl == NULL) > + caml_raise_out_of_memory(); > + > + memset(xtl, 0, sizeof(*xtl)); > + > + xtl->vtable.vmessage = &stub_xtl_ocaml_vmessage; > + xtl->vtable.progress = &stub_xtl_ocaml_progress; > + xtl->vtable.destroy = &xtl_destroy; > + > + xtl->vmessage_cb = dup_String_val(Field(cbs, 0)); > + xtl->progress_cb = dup_String_val(Field(cbs, 1)); > + CAMLreturn((value)xtl); > +} I think we should avoid returning "bare pointers" to the OCaml heap for two reasons: Firstly it makes us vulnerable to a sequence like the following: 1. malloc() something out of heap 2. return the "bare pointer" to the heap 3. GC runs, ignores the bare pointer because it points out of heap ... some time later ... 4. we call free() on the out of heap thing ... some time later ... 5. GC runs, ignores the bare pointer because it points out of heap ... some time and many heap allocations later ... 6. GC expands <-- the heap now includes the old address used by the bare pointer 7. GC runs, follows the bare pointer because it points inside the heap and segfaults Secondly it prevents some heap optimisations that are being experimented with by people at OCamlLabs, so we'd be storing up some incompatibility for the future. Instead of returning a "bare pointer" I think we should use a "Custom" value. This involves declaring a "struct custom_operations" like this: static struct custom_operations foo_custom_operations = { "foo_custom_operations", custom_finalize_default, custom_compare_default, custom_hash_default, custom_serialize_default, custom_deserialize_default }; And then wrapping and unwrapping "Custom" blocks using something like: #define Foo_val(x) (*((struct foo *)Data_custom_val(x))) static value Val_foo (struct foo *x) { CAMLparam0 (); CAMLlocal1 (result); result = caml_alloc_custom (&foo_custom_operations, sizeof (struct foo*), 0, 1); Foo_val (result) = x; CAMLreturn (result); } There's more information here: http://caml.inria.fr/pub/docs/manual-ocaml-4.00/manual033.html#toc150 The ocaml-libvirt bindings are a good example of this pattern: http://git.annexia.org/?p=ocaml-libvirt.git;a=summary It's also worth considering whether to use the finalizer support to automatically free the underlying C resource when the last reference to it from OCaml has been GCed. This would be safer than exposing a direct "free" function in the OCaml interface, since it would prevent use-after-free. Cheers, Dave