[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 08 of 15] libxl/ocaml: add some more builtin types



On Thu, 2012-11-29 at 17:19 +0000, Rob Hoes wrote:
> > # HG changeset patch
> > # User Ian Campbell <ijc@xxxxxxxxxxxxxx> # Date 1353432141 0 # Node ID
> > 21c5e58956d09437903e1ee1b0588d61a7c28145
> > # Parent  0cf342afa9e6b506fad68346cb3a1207030372eb
> > libxl/ocaml: add some more builtin types.
> > 
> >   * bitmaps
> >   * string_list
> >   * cpuid_policy_list (actually opaque)
> >   * key_value_list
> > 
> > None of these are used yet, so no change to the generated code.
> > 
> > Bitmap_val requires a ctx, so leave it as an abort for now.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > diff -r 0cf342afa9e6 -r 21c5e58956d0 tools/ocaml/libs/xl/genwrap.py
> > --- a/tools/ocaml/libs/xl/genwrap.py        Tue Nov 20 17:22:21 2012 +0000
> > +++ b/tools/ocaml/libs/xl/genwrap.py        Tue Nov 20 17:22:21 2012 +0000
> > @@ -13,9 +13,13 @@ builtins = {
> >      "libxl_devid":          ("devid",                  "%(c)s = 
> > Int_val(%(o)s)",
> > "Val_int(%(c)s)"  ),
> >      "libxl_defbool":        ("bool option",            "%(c)s = 
> > Defbool_val(%(o)s)",
> > "Val_defbool(%(c)s)" ),
> >      "libxl_uuid":           ("int array",              "Uuid_val(gc, lg, 
> > &%(c)s, %(o)s)",
> > "Val_uuid(&%(c)s)"),
> > -    "libxl_key_value_list": ("(string * string) list", None,
> > None),
> > +    "libxl_bitmap":         ("bool array",             "Bitmap_val(gc, lg,
> > &%(c)s, %(o)s)",   "Val_bitmap(&%(c)s)"),
> > +    "libxl_key_value_list": ("(string * string) list",
> > "libxl_key_value_list_val(gc, lg, &%(c)s, %(o)s)",                          
> >     None),
> > +    "libxl_string_list":    ("string list",            
> > "libxl_string_list_val(gc, lg,
> > &%(c)s, %(o)s)",                                 "String_list_val(gc, lg, 
> > &%(c)s, %(o)s)"),
> >      "libxl_mac":            ("int array",              "Mac_val(gc, lg, 
> > &%(c)s, %(o)s)",
> > "Val_mac(&%(c)s)"),
> >      "libxl_hwcap":          ("int32 array",            None,
> > "Val_hwcap(&%(c)s)"),
> > +    # This is an opaque type
> > +    "libxl_cpuid_policy_list": ("Cpuid_policy.t",      
> > "Cpuid_policy_list_val(gc,
> > lg, &%(c)s, %(o)s)",   "Val_cpuid_policy_list(&%(c)s)"),
> >      }
> > 
> >  DEVICE_FUNCTIONS = [ ("add",            ["t", "domid", "unit"]),
> > diff -r 0cf342afa9e6 -r 21c5e58956d0 tools/ocaml/libs/xl/xenlight.ml.in
> > --- a/tools/ocaml/libs/xl/xenlight.ml.in    Tue Nov 20 17:22:21 2012
> > +0000
> > +++ b/tools/ocaml/libs/xl/xenlight.ml.in    Tue Nov 20 17:22:21 2012
> > +0000
> > @@ -18,6 +18,10 @@ exception Error of string  type domid = int  type devid
> > = int
> > 
> > +module Cpuid_policy = struct
> > +   type t
> > +end
> > +
> 
> Do you expect this type to become more complicated, or non-opaque, in future?

IIRC it is opaque at the libxc layer too and I don't see this changing.
Mostly because its a total brain bender ;-)

>  Or would it have functions associated with it like for the devices?

I think it will eventually get functions to initialise the opaque thing,
corresponding to one or more of:

int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str);
int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
                                  const char* str);
void libxl_cpuid_apply_policy(libxl_ctx *ctx, uint32_t domid);
void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
                     libxl_cpuid_policy_list cpuid);

or some new accessor useful to you guys.

>  If not, perhaps we can use a simpler type definition:
> 
> type cpuid_policy_list
> 
> >  (* @@LIBXL_TYPES@@ *)
> > 
> >  external send_trigger : domid -> trigger -> int -> unit =
> > "stub_xl_send_trigger"
> > diff -r 0cf342afa9e6 -r 21c5e58956d0 tools/ocaml/libs/xl/xenlight_stubs.c
> > --- a/tools/ocaml/libs/xl/xenlight_stubs.c  Tue Nov 20 17:22:21 2012
> > +0000
> > +++ b/tools/ocaml/libs/xl/xenlight_stubs.c  Tue Nov 20 17:22:21 2012
> > +0000
> > @@ -27,6 +27,7 @@
> >  #include <string.h>
> > 
> >  #include <libxl.h>
> > +#include <libxl_utils.h>
> > 
> >  struct caml_logger {
> >     struct xentoollog_logger logger;
> > @@ -96,7 +97,6 @@ static void failwith_xl(char *fname, str
> >     caml_raise_with_string(*caml_named_value("xl.error"), s);  }
> > 
> > -#if 0 /* TODO: wrap libxl_domain_create(), these functions will be needed
> > then */  static void * gc_calloc(caml_gc *gc, size_t nmemb, size_t size)  {
> >     void *ptr;
> > @@ -107,28 +107,62 @@ static void * gc_calloc(caml_gc *gc, siz
> >     return ptr;
> >  }
> > 
> > -static int string_string_tuple_array_val (caml_gc *gc, char ***c_val, 
> > value v)
> > +static int list_len(value v)
> > +{
> > +   int len = 0;
> > +   while ( v != Val_emptylist ) {
> > +           len++;
> > +           v = Field(v, 1);
> > +   }
> > +   return len;
> > +}
> > +
> 
> It is probably best to use CAMLparam1(v) and CAMLreturn(len) here, just in 
> case.

Yes, thanks.

> 
> > +static int libxl_key_value_list_val(caml_gc *gc, struct caml_logger *lg,
> > +                               libxl_key_value_list *c_val,
> > +                               value v)
> >  {
> >     CAMLparam1(v);
> > -   CAMLlocal1(a);
> > -   int i;
> > -   char **array;
> > +   CAMLlocal1(elem);
> > +   int nr, i;
> > +   libxl_key_value_list array;
> > 
> > -   for (i = 0, a = Field(v, 5); a != Val_emptylist; a = Field(a, 1)) { 
> > i++; }
> > +   nr = list_len(v);
> > 
> > -   array = gc_calloc(gc, (i + 1) * 2, sizeof(char *));
> > +   array = gc_calloc(gc, (nr + 1) * 2, sizeof(char *));
> >     if (!array)
> > -           return 1;
> > -   for (i = 0, a = Field(v, 5); a != Val_emptylist; a = Field(a, 1), i++) {
> > -           value b = Field(a, 0);
> > -           array[i * 2] = dup_String_val(gc, Field(b, 0));
> > -           array[i * 2 + 1] = dup_String_val(gc, Field(b, 1));
> > +           caml_raise_out_of_memory();
> > +
> > +   for (i=0; v != Val_emptylist; i++, v = Field(v, 1) ) {
> > +           elem = Field(v, 0);
> > +
> > +           array[i * 2] = dup_String_val(gc, Field(elem, 0));
> > +           array[i * 2 + 1] = dup_String_val(gc, Field(elem, 1));
> >     }
> > +
> >     *c_val = array;
> >     CAMLreturn(0);
> >  }
> > 
> > -#endif
> > +static int libxl_string_list_val(caml_gc *gc, struct caml_logger *lg,
> > +                            libxl_string_list *c_val,
> > +                            value v)
> > +{
> > +   CAMLparam1(v);
> > +   int nr, i;
> > +   libxl_key_value_list array;
> 
> This should probably be a libxl_string_list.

Yes!

(they are actually the same type under the hood, which is why gcc
doesn't complain)

> 
> > +
> > +   nr = list_len(v);
> > +
> > +   array = gc_calloc(gc, (nr + 1), sizeof(char *));
> > +   if (!array)
> > +           caml_raise_out_of_memory();
> > +
> > +   for (i=0; v != Val_emptylist; i++, v = Field(v, 1) )
> > +           array[i] = dup_String_val(gc, Field(v, 0));
> > +
> > +   *c_val = array;
> > +   CAMLreturn(0);
> > +}
> > 
> >  /* Option type support as per http://www.linux-
> > nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */  #define Val_none
> > Val_int(0) @@ -168,6 +202,45 @@ static int Mac_val(caml_gc *gc, struct c
> >     CAMLreturn(0);
> >  }
> > 
> > +static value Val_bitmap (libxl_bitmap *c_val) {
> > +   CAMLparam0();
> > +   CAMLlocal1(v);
> > +   int i;
> > +
> > +   v = caml_alloc(8 * (c_val->size), 0);
> > +   libxl_for_each_bit(i, *c_val) {
> > +           if (libxl_bitmap_test(c_val, i))
> > +                   Store_field(v, i, Val_true);
> > +           else
> > +                   Store_field(v, i, Val_false);
> > +   }
> > +   CAMLreturn(v);
> > +}
> > +
> > +static int Bitmap_val(caml_gc *gc, struct caml_logger *lg,
> > +                 libxl_bitmap *c_val, value v)
> > +{
> > +   abort(); /* XXX */
> > +}
> > +
> > +static value Val_cpuid_policy_list(libxl_cpuid_policy_list *c_val) {
> > +   CAMLparam0();
> > +   /* An opaque pointer */
> > +   CAMLreturn((value)c_val);
> > +}
> > +
> > +static int Cpuid_policy_list_val(caml_gc *gc, struct caml_logger *lg,
> > +                            libxl_cpuid_policy_list **c_val, value v) {
> > +   CAMLparam1(v);
> > +
> > +   /* An opaque pointer */
> > +   *c_val = (libxl_cpuid_policy_list*)v;
> > +   CAMLreturn(0);
> > +}
> > +
> >  static value Val_uuid (libxl_uuid *c_val)  {
> >     CAMLparam0();
> 
> For the rest it looks good to me.
> 
> Cheers,
> Rob
> 



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.