From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Scott <dave.scott@eu.citrix.com>
Subject: Re: [PATCH 16/28] libxl: ocaml: use the "string option"
 type for IDL strings
Date: Tue, 26 Mar 2013 11:43:56 +0000
Message-ID: <515189FC.9020805@eu.citrix.com>
References: <1364222729-6982-1-git-send-email-rob.hoes@citrix.com>
	<1364222729-6982-17-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: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1364222729-6982-17-git-send-email-rob.hoes@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Rob Hoes <rob.hoes@citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On 25/03/13 14:45, Rob Hoes wrote:
[ snip ]
> +static value Val_string_option(char *c_val)
> +{
> +	CAMLparam0();
> +	if (c_val)
> +		CAMLreturn(Val_some(caml_copy_string(c_val)));

A bad sequence is:

   1. caml_copy_string() allocates a string successfully
   2. Val_some() calls the allocator to allocate a block but the minor 
heap is full so it triggers a GC
   3. the GC deletes the string from (1) since it can't find any 
references to it

Personally I always force myself to write very basic code with lots of 
explicit temporaries, just to be totally safe. It feels strange because 
it's the complete opposite of good functional style (particularly if you 
believe in point-free programming!).

So I would write:

   CAMLparam0()
   CAMLlocal2(tmp1, tmp2)
   if (c_val) {
     tmp1 = caml_copy_string(c_val);
     tmp2 = Val_some(tmp1);
     CAMLreturn(tmp2)
   }
   ...

It's almost embarrassing to write code like that, but at least it's 
safe! :-)

> +	else
> +		CAMLreturn(Val_none);
> +}
> +
> +static char *String_option_val(value v)
> +{
> +	char *s = NULL;
> +	if (v != Val_none)
> +		s = dup_String_val(Some_val(v));
> +	return s;
> +}
> +
>   #include "_libxl_types.inc"
>
>   #define _STRINGIFY(x) #x
>