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

[Xen-devel] Re: [PATCH, v3]: change libxl IDL to export a saner interface for upcoming language bindings



On Fri, 2010-09-10 at 16:41 +0100, Gianni Tedesco wrote:
> On Fri, 2010-09-10 at 16:40 +0100, Gianni Tedesco wrote:
> > Since version 2:
> >  - Fold in Ian Campbells patch tidying formatting and to to make
> >    autogenerate_destructor=False by default
> > Since version 1:
> >  - Fix endian problem with removal of pcidev->value anon union member
> 
> FFS, forgot to actually update the patch! What is described above, now
> reproduced below!

:-D

> 
> ---8<------------------------------------------------------------
> Firstly remove an anonymous union in libxl_device_pci structure which
> was making auto-generating language bindings more complicated than
> necessary and exporting random bits of low level ABI that libxl that
> would rather hide anyway. There is a corresponding (untested) change to
> the ocaml binding which maintains previous ml API.
> 
> Secondly make the libxl_file_reference type a Builtin. This is a
> 'semantic
> correctness' issue in that libxl ABI/API won't change. But it makes it
> so that when the IDL is used to generate language bindings that a
> file_reference type is not exported.
> 
> Also implement a Numeric type which all integers are derived from. Make
> sure a boolean signed/unsigned attribute is set accordingly. This is
> required to allow language bindings to correctly handle the sign bit in
> environments with arbitrarily long integers.
> 
> Signed-off-by: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> 
> diff -r 4137ca280d14 tools/libxl/idl.txt
> --- a/tools/libxl/idl.txt     Fri Sep 10 15:39:33 2010 +0100
> +++ b/tools/libxl/idl.txt     Fri Sep 10 16:31:07 2010 +0100
> @@ -13,7 +13,8 @@ contain the initial namespace element (e
>  how to specify a namespace.
>  
>  The Type.typename contains the C name of the type _including_ the
> -namespace element.
> +namespace element while Type.rawname is always set to the 'base' name
> +of the type.
>  
>  The libxltypes.Type base class has several other properties which
>  apply to all types. The properties are set by passing a named
> diff -r 4137ca280d14 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h     Fri Sep 10 15:39:33 2010 +0100
> +++ b/tools/libxl/libxl.h     Fri Sep 10 16:31:07 2010 +0100
> @@ -136,8 +136,10 @@
>  typedef uint8_t libxl_mac[6];
>  
>  typedef char **libxl_string_list;
> +void libxl_string_list_destroy(libxl_string_list *sl);
>  
>  typedef char **libxl_key_value_list;
> +void libxl_key_value_list_destroy(libxl_key_value_list *kvl);
>  
>  typedef uint64_t *libxl_cpumap;
>  
> @@ -172,6 +174,18 @@ typedef enum {
>      NICTYPE_VIF,
>  } libxl_nic_type;
>  
> +typedef struct {
> +    /*
> +     * Path is always set if the file reference is valid. However if
> +     * mapped is true then the actual file may already be unlinked.
> +     */
> +    char * path;
> +    int mapped;
> +    void * data;
> +    size_t size;
> +} libxl_file_reference;
> +void libxl_file_reference_destroy(libxl_file_reference *p);
> +
>  #define LIBXL_PCI_FUNC_ALL (~0U)
>  
>  #include "_libxl_types.h"
> @@ -227,11 +241,6 @@ int libxl_domain_shutdown(libxl_ctx *ctx
>  int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, int force);
>  int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, 
> libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
>  
> -/* destructors for builtin data types */
> -void libxl_string_list_destroy(libxl_string_list *sl);
> -void libxl_key_value_list_destroy(libxl_key_value_list *kvl);
> -void libxl_file_reference_destroy(libxl_file_reference *f);
> -
>  /*
>   * Run the configured bootloader for a PV domain and update
>   * info->kernel, info->u.pv.ramdisk and info->u.pv.cmdline as
> diff -r 4137ca280d14 tools/libxl/libxl.idl
> --- a/tools/libxl/libxl.idl   Fri Sep 10 15:39:33 2010 +0100
> +++ b/tools/libxl/libxl.idl   Fri Sep 10 16:31:07 2010 +0100
> @@ -6,14 +6,15 @@
>  libxl_ctx = Builtin("ctx")
>  libxl_uuid = Builtin("uuid")
>  libxl_mac = Builtin("mac")
> -libxl_qemu_machine_type = Builtin("qemu_machine_type")
> -libxl_console_consback = Builtin("console_consback")
> -libxl_console_constype = Builtin("console_constype")
> -libxl_disk_phystype = Builtin("disk_phystype")
> -libxl_nic_type = Builtin("nic_type")
> +libxl_qemu_machine_type = Number("qemu_machine_type", namespace="libxl_")
> +libxl_console_consback = Number("console_consback", namespace="libxl_")
> +libxl_console_constype = Number("console_constype", namespace="libxl_")
> +libxl_disk_phystype = Number("disk_phystype", namespace="libxl_")
> +libxl_nic_type = Number("nic_type", namespace="libxl_")
>  
>  libxl_string_list = Builtin("string_list", 
> destructor_fn="libxl_string_list_destroy", passby=PASS_BY_REFERENCE)
>  libxl_key_value_list = Builtin("key_value_list", 
> destructor_fn="libxl_key_value_list_destroy", passby=PASS_BY_REFERENCE)
> +libxl_file_reference = Builtin("file_reference", 
> destructor_fn="libxl_file_reference_destroy", passby=PASS_BY_REFERENCE)
>  
>  libxl_cpumap = Builtin("cpumap", destructor_fn="free")
>  
> @@ -79,14 +80,6 @@ libxl_domain_create_info = Struct("domai
>      ("poolname",     string),
>      ])
>  
> -libxl_file_reference = Struct("file_reference",[
> -    ("path", string, False,
> -"""Path is always set if the file reference is valid. However if
> -mapped is true then the actual file may already be unlinked."""),
> -    ("mapped", integer),
> -    ("data", void),
> -    ("size", size_t)], autogenerate_destructor=False)
> -
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("cur_vcpus",       integer),
> @@ -240,17 +233,11 @@ libxl_device_net2 = Struct("device_net2"
>      ])
>  
>  libxl_device_pci = Struct("device_pci", [
> -    (None, Union(None, [("value", unsigned_integer),
> -                        (None, Struct(None,[("reserved1", 
> BitField(unsigned_integer, 2)),
> -                                            ("reg",       
> BitField(unsigned_integer, 6)),
> -                                            ("func",      
> BitField(unsigned_integer, 3)),
> -                                            ("dev",       
> BitField(unsigned_integer, 5)),
> -                                            ("bus",       
> BitField(unsigned_integer, 8)),
> -                                            ("reserved2", 
> BitField(unsigned_integer, 7)),
> -                                            ("enable",    
> BitField(unsigned_integer, 1)),
> -                                             ])),
> -                        ])
> -     ),
> +    ("reg",       uint8),
> +    ("func",      uint8),
> +    ("dev",       uint8),
> +    ("bus",       uint8),
> +    ("enable",    bool),
>      ("domain", unsigned_integer),
>      ("vdevfn", unsigned_integer),
>      ("vfunc_mask", unsigned_integer),
> diff -r 4137ca280d14 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c Fri Sep 10 15:39:33 2010 +0100
> +++ b/tools/libxl/libxl_pci.c Fri Sep 10 16:31:07 2010 +0100
> @@ -41,6 +41,31 @@
>  #define PCI_BDF_SHORT          "%02x:%02x.%01x"
>  #define PCI_BDF_VDEVFN         "%04x:%02x:%02x.%01x@%02x"
>  
> +static unsigned int pcidev_value(libxl_device_pci *pcidev)
> +{
> +    union {
> +        unsigned int value;
> +        struct {
> +            unsigned int reserved1:2;
> +            unsigned int reg:6;
> +            unsigned int func:3;
> +            unsigned int dev:5;
> +            unsigned int bus:8;
> +            unsigned int reserved2:7;
> +            unsigned int enable:1;
> +        }fields;
> +    }u;
> +
> +    u.value = 0;
> +    u.fields.reg = pcidev->reg;
> +    u.fields.func = pcidev->func;
> +    u.fields.dev = pcidev->dev;
> +    u.fields.bus = pcidev->bus;
> +    u.fields.enable = pcidev->enable;
> +
> +    return u.value;
> +}
> +
>  static int pcidev_init(libxl_device_pci *pcidev, unsigned int domain,
>                            unsigned int bus, unsigned int dev,
>                            unsigned int func, unsigned int vdevfn)
> @@ -699,7 +724,7 @@ static int do_pci_add(libxl__gc *gc, uin
>      }
>  out:
>      if (!libxl_is_stubdom(ctx, domid, NULL)) {
> -        rc = xc_assign_device(ctx->xch, domid, pcidev->value);
> +        rc = xc_assign_device(ctx->xch, domid, pcidev_value(pcidev));
>          if (rc < 0 && (hvm || errno != ENOSYS)) {
>              LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, "xc_assign_device 
> failed");
>              return ERROR_FAIL;
> @@ -915,7 +940,7 @@ out:
>      }
>  
>      if (!libxl_is_stubdom(ctx, domid, NULL)) {
> -        rc = xc_deassign_device(ctx->xch, domid, pcidev->value);
> +        rc = xc_deassign_device(ctx->xch, domid, pcidev_value(pcidev));
>          if (rc < 0 && (hvm || errno != ENOSYS))
>              LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, 
> "xc_deassign_device failed");
>      }
> diff -r 4137ca280d14 tools/libxl/libxltypes.py
> --- a/tools/libxl/libxltypes.py       Fri Sep 10 15:39:33 2010 +0100
> +++ b/tools/libxl/libxltypes.py       Fri Sep 10 16:31:07 2010 +0100
> @@ -14,10 +14,13 @@ class Type(object):
>  
>          if typename is None: # Anonymous type
>              self.typename = None
> +            self.rawname = None
>          elif self.namespace is None: # e.g. system provided types
>              self.typename = typename
> +            self.rawname = typename
>          else:
>              self.typename = self.namespace + typename
> +            self.rawname = typename
>  
>          if self.typename is not None:
>              self.destructor_fn = kwargs.setdefault('destructor_fn', 
> self.typename + "_destroy")
> @@ -30,13 +33,22 @@ class Builtin(Type):
>      """Builtin type"""
>      def __init__(self, typename, **kwargs):
>          kwargs.setdefault('destructor_fn', None)
> +        kwargs.setdefault('autogenerate_destructor', False)
>          Type.__init__(self, typename, **kwargs)
>  
> -class UInt(Type):
> +class Number(Builtin):
> +    def __init__(self, ctype, **kwargs):
> +        kwargs.setdefault('namespace', None)
> +        kwargs.setdefault('destructor_fn', None)
> +        kwargs.setdefault('signed', False)
> +        self.signed = kwargs['signed']
> +        Builtin.__init__(self, ctype, **kwargs)
> +
> +class UInt(Number):
>      def __init__(self, w, **kwargs):
>          kwargs.setdefault('namespace', None)
>          kwargs.setdefault('destructor_fn', None)
> -        Type.__init__(self, "uint%d_t" % w, **kwargs)
> +        Number.__init__(self, "uint%d_t" % w, **kwargs)
>  
>          self.width = w
>  
> @@ -128,12 +140,12 @@ class Reference(Type):
>  
>  void = Builtin("void *", namespace = None)
>  bool = Builtin("bool", namespace = None)
> -size_t = Builtin("size_t", namespace = None)
> +size_t = Number("size_t", namespace = None)
>  
> -integer = Builtin("int", namespace = None)
> -unsigned_integer = Builtin("unsigned int", namespace = None)
> -unsigned = Builtin("unsigned int", namespace = None)
> -unsigned_long = Builtin("unsigned long", namespace = None)
> +integer = Number("int", namespace = None, signed = True)
> +unsigned_integer = Number("unsigned int", namespace = None)
> +unsigned = Number("unsigned int", namespace = None)
> +unsigned_long = Number("unsigned long", namespace = None)
>  
>  uint8 = UInt(8)
>  uint16 = UInt(16)
> diff -r 4137ca280d14 tools/ocaml/libs/xl/xl_stubs.c
> --- a/tools/ocaml/libs/xl/xl_stubs.c  Fri Sep 10 15:39:33 2010 +0100
> +++ b/tools/ocaml/libs/xl/xl_stubs.c  Fri Sep 10 16:31:07 2010 +0100
> @@ -269,9 +269,28 @@ static int device_vfb_val(caml_gc *gc, l
>  
>  static int device_pci_val(caml_gc *gc, libxl_device_pci *c_val, value v)
>  {
> +     union {
> +             unsigned int value;
> +             struct {
> +                     unsigned int reserved1:2;
> +                     unsigned int reg:6;
> +                     unsigned int func:3;
> +                     unsigned int dev:5;
> +                     unsigned int bus:8;
> +                     unsigned int reserved2:7;
> +                     unsigned int enable:1;
> +             }fields;
> +     }u;
>       CAMLparam1(v);
>  
> -     c_val->value = Int_val(Field(v, 0));
> +     /* FIXME: propagate API change to ocaml */
> +     u.value = Int_val(Field(v, 0));
> +     c_val->reg = u.fields.reg;
> +     c_val->func = u.fields.func;
> +     c_val->dev = u.fields.dev;
> +     c_val->bus = u.fields.bus;
> +     c_val->enable = u.fields.enable;
> +
>       c_val->domain = Int_val(Field(v, 1));
>       c_val->vdevfn = Int_val(Field(v, 2));
>       c_val->msitranslate = Bool_val(Field(v, 3));
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

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