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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

Ian Campbell wrote:
> 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(...);

Right, that's a typo while reorganising my patches... I'll send an update 
shortly.

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

It doesn't need to be. It just made the function a little nicer to use (to me). 
I appreciate that C-experts may have better ways of doing this :)

> >             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)

Ok, good. I'll update that together with the fix above.

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