WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH, v3]: change libxl IDL to export a saner interfac

To: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH, v3]: change libxl IDL to export a saner interface for upcoming language bindings
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Fri, 10 Sep 2010 16:56:02 +0100
Cc: Vincent Hanquez <Vincent.Hanquez@xxxxxxxxxxxxx>, Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Delivery-date: Fri, 10 Sep 2010 08:56:43 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1284133315.17452.165.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1284133203.17452.163.camel@xxxxxxxxxxxxxxxxxxxxxx> <1284133315.17452.165.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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