Re: [Xen-devel] [PATCH v6 04/11] libxl: ocaml: allow device operations to be called asynchronously

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-12-10 at 13:25 +0000, Ian Campbell wrote:
> On Mon, 2013-12-09 at 15:17 +0000, Rob Hoes wrote:
> > Signed-off-by: Rob Hoes <rob.hoes@xxxxxxxxxx>
> > CC: David Scott <dave.scott@xxxxxxxxxxxxx>
> > CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > ---
> > v6: Updated patch to register for_callback value with the GC.
> > ---
> >  tools/ocaml/libs/xl/genwrap.py       |    6 +++---
> >  tools/ocaml/libs/xl/xenlight_stubs.c |   18 +++++++++++++++---
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
> > index f5d2224..dd43069 100644
> > --- a/tools/ocaml/libs/xl/genwrap.py
> > +++ b/tools/ocaml/libs/xl/genwrap.py
> > @@ -22,9 +22,9 @@ builtins = {
> >      "libxl_cpuid_policy_list": ("unit",                "%(c)s = 0",        
> >                  "Val_unit"),
> >      }
> >  
> > -DEVICE_FUNCTIONS = [ ("add",            ["ctx", "t", "domid", "unit"]),
> > -                     ("remove",         ["ctx", "t", "domid", "unit"]),
> > -                     ("destroy",        ["ctx", "t", "domid", "unit"]),
> > +DEVICE_FUNCTIONS = [ ("add",            ["ctx", "t", "domid", "?async:'a", 
> > "unit", "unit"]),
> > +                     ("remove",         ["ctx", "t", "domid", "?async:'a", 
> > "unit", "unit"]),
> > +                     ("destroy",        ["ctx", "t", "domid", "?async:'a", 
> > "unit", "unit"]),
> >                     ]
> >  
> >  functions = { # ( name , [type1,type2,....] )
> > diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
> > b/tools/ocaml/libs/xl/xenlight_stubs.c
> > index 660dd09..b6649a7 100644
> > --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> > +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> > @@ -401,15 +401,27 @@ void async_callback(libxl_ctx *ctx, int rc, void 
> > *for_callback)
> >  #define STRINGIFY(x) _STRINGIFY(x)
> >  
> >  #define _DEVICE_ADDREMOVE(type,op)                                 \
> > -value stub_xl_device_##type##_##op(value ctx, value info, value domid)     
> > \
> > +value stub_xl_device_##type##_##op(value ctx, value info, value domid,     
> > \
> > +   value async, value unit)                                        \
> >  {                                                                  \
> > -   CAMLparam3(ctx, info, domid);                                   \
> > +   CAMLparam5(ctx, info, domid, async, unit);                      \
> >     libxl_device_##type c_info;                                     \
> >     int ret, marker_var;                                            \
> > +   libxl_asyncop_how ao_how;                                       \
> > +   value *p;                                                       \
> >                                                                     \
> >     device_##type##_val(CTX, &c_info, info);                        \
> >                                                                     \
> > -   ret = libxl_device_##type##_##op(CTX, Int_val(domid), &c_info, 0); \
> > +   if (async != Val_none) {                                        \
> > +           p = malloc(sizeof(value));                              \
> 
> This lacks error handling, or as Ian J suggested previously a wrapper to
> fail in a graceful manner. I think that can be a future more global
> cleanup though.

I see in a subsequent patch some similar code gets:
+               if (!p)
+                       failwith_xl(ERROR_NOMEM, "cannot allocate value");

Would be worth having here too.

> 
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> > +           *p = Some_val(async);                                   \
> > +           caml_register_global_root(p);                           \
> > +           ao_how.callback = async_callback;                       \
> > +           ao_how.u.for_callback = (void *) p;                     \
> > +   }                                                               \
> > +                                                                   \
> > +   ret = libxl_device_##type##_##op(CTX, Int_val(domid), &c_info,  \
> > +           async != Val_none ? &ao_how : NULL);                    \
> >                                                                     \
> >     libxl_device_##type##_dispose(&c_info);                         \
> >                                                                     \
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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