Re: [Xen-devel] [PATCH v6 08/11] libxl: ocaml: drop the ocaml heap lock before calling into libxl

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-09 at 15:17 +0000, Rob Hoes wrote:
> }
>  
> -static libxl_asyncop_how *aohow_val(value async, libxl_asyncop_how *ao_how)
> +static libxl_asyncop_how *aohow_val(value async)
>  {
>       CAMLparam1(async);
> +     libxl_asyncop_how *ao_how = NULL;
>       value *p;
>  
>       if (async != Val_none) {
> @@ -408,12 +418,12 @@ static libxl_asyncop_how *aohow_val(value async, 
> libxl_asyncop_how *ao_how)
>                       failwith_xl(ERROR_NOMEM, "cannot allocate value");
>               *p = Some_val(async);
>               caml_register_global_root(p);
> +             libxl_asyncop_how *ao_how = malloc(sizeof(*ao_how));

This will shadow the definition at the top of the function, meaning this
function always returns NULL AFAICT. I think you meant just
                ao_how = malloc(...);

Can you remind me why this needs to be dynamically allocated please,
preferably by adding the explanation to the commit message.

>               ao_how->callback = async_callback;
>               ao_how->u.for_callback = (void *) p;
> -             CAMLreturnT(libxl_asyncop_how *, ao_how);
>       }
> -     else
> -             CAMLreturnT(libxl_asyncop_how *, NULL);
> +
> +     CAMLreturnT(libxl_asyncop_how *, ao_how);
>  }
>  
>  value stub_libxl_domain_create_new(value ctx, value domain_config, value 
> async, value unit)
> @@ -422,7 +432,7 @@ value stub_libxl_domain_create_new(value ctx, value 
> domain_config, value async,
>       int ret;
>       libxl_domain_config c_dconfig;
>       uint32_t c_domid;
> -     libxl_asyncop_how ao_how;
> +     libxl_asyncop_how *ao_how;
>  
>       libxl_domain_config_init(&c_dconfig);
>       ret = domain_config_val(CTX, &c_dconfig, domain_config);
> @@ -431,9 +441,14 @@ value stub_libxl_domain_create_new(value ctx, value 
> domain_config, value async,
>               failwith_xl(ret, "domain_create_new");
>       }
>  
> -     ret = libxl_domain_create_new(CTX, &c_dconfig, &c_domid,
> -             aohow_val(async, &ao_how), NULL);
> +     ao_how = aohow_val(async);
> +
> +     caml_enter_blocking_section();
> +     ret = libxl_domain_create_new(CTX, &c_dconfig, &c_domid, ao_how, NULL);
> +     caml_leave_blocking_section();
>  
> +     if (ao_how)
> +             free(ao_how);

free(NULL) is fine, so you can avoid the if. (multiple times in this
patch)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel