thr3ads.net - Xen devel - [Xen-devel] [PATCH]: change libxl IDL to export a saner interface for upcoming language bindings [Sep 2010]

If this information is useful, please help other people find it:
Share via:

Gianni Tedesco

2010-Sep-09 13:07 UTC

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

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.

Also 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.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r b19856f6dd76 tools/libxl/idl.txt
--- a/tools/libxl/idl.txt	Thu Sep 09 09:24:24 2010 +0100
+++ b/tools/libxl/idl.txt	Thu Sep 09 13:52:46 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 b19856f6dd76 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Sep 09 09:24:24 2010 +0100
+++ b/tools/libxl/libxl.h	Thu Sep 09 13:52:46 2010 +0100
@@ -172,6 +172,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"
diff -r b19856f6dd76 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Thu Sep 09 09:24:24 2010 +0100
+++ b/tools/libxl/libxl.idl	Thu Sep 09 13:52:46 2010 +0100
@@ -6,11 +6,11 @@
 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)
@@ -79,13 +79,10 @@ 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_file_reference = Builtin("file_reference",
+                               autogenerate_destructor=False,
+                              
destructor_fn="libxl_file_reference_destroy",
+                               passby=PASS_BY_REFERENCE)
 
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
@@ -240,17 +237,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 b19856f6dd76 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Thu Sep 09 09:24:24 2010 +0100
+++ b/tools/libxl/libxl_pci.c	Thu Sep 09 13:52:46 2010 +0100
@@ -41,6 +41,15 @@
 #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)
+{
+    return ((pcidev->reg & 0x3f) << 24) |
+           ((pcidev->func & 0x7) << 21) |
+           ((pcidev->dev & 0x1f) << 16) |
+           ((pcidev->bus & 0xff) << 8) |
+           ((pcidev->enable) ? 1 : 0);
+}
+
 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 +708,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 +924,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 b19856f6dd76 tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Thu Sep 09 09:24:24 2010 +0100
+++ b/tools/libxl/libxltypes.py	Thu Sep 09 13:52:46 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")
@@ -32,11 +35,19 @@ class Builtin(Type):
         kwargs.setdefault(''destructor_fn'', None)
         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 +139,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 b19856f6dd76 tools/ocaml/libs/xl/xl_stubs.c
--- a/tools/ocaml/libs/xl/xl_stubs.c	Thu Sep 09 09:24:24 2010 +0100
+++ b/tools/ocaml/libs/xl/xl_stubs.c	Thu Sep 09 13:52:46 2010 +0100
@@ -269,9 +269,17 @@ static int device_vfb_val(caml_gc *gc, l
 
 static int device_pci_val(caml_gc *gc, libxl_device_pci *c_val, value v)
 {
+	unsigned int bitmask;
 	CAMLparam1(v);
 
-	c_val->value = Int_val(Field(v, 0));
+	/* FIXME: propagate API change to ocaml */
+	bitmask = Int_val(Field(v, 0));
+	c_val->reg = (bitmask & (0x3f << 24)) >> 24;
+	c_val->func = (bitmask & (0x7 << 21)) >> 21;
+	c_val->dev = (bitmask & (0x1f << 16)) >> 16;
+	c_val->bus = (bitmask & (0xff << 8)) >> 8;
+	c_val->enable = bitmask & 1;
+
 	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@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Sep-09 13:37 UTC

head link

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

On Thu, 2010-09-09 at 14:07 +0100, Gianni Tedesco wrote:> 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.
> 
> Also 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.
You forgot to mention the addition of the Numbers type.

Otherwise:

Acked-by: Ian Campbell <ian.campbell@citrix.com>
> -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_file_reference = Builtin("file_reference",
> +                               autogenerate_destructor=False,
> +                              
destructor_fn="libxl_file_reference_destroy",
> +                               passby=PASS_BY_REFERENCE)
autogenerate_destructor=False doesn''t do much on a Builtin in any case
since we never generate destructors for builtins. It should probably
just be a default in the base type.

i.e. cumulatively on your patch:

Subject: libxl: Builtin types default to autogenerate_destructor=False

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r fe1050a60d8f -r c2b17bd27d7d tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Thu Sep 09 14:32:34 2010 +0100
+++ b/tools/libxl/libxl.idl	Thu Sep 09 14:34:45 2010 +0100
@@ -14,6 +14,7 @@ libxl_nic_type = Number("nic_type", name
 
 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")
 
@@ -78,11 +79,6 @@ libxl_domain_create_info = Struct("domai
     ("poolid",       uint32),
     ("poolname",     string),
     ])
-
-libxl_file_reference = Builtin("file_reference",
-                               autogenerate_destructor=False,
-                              
destructor_fn="libxl_file_reference_destroy",
-                               passby=PASS_BY_REFERENCE)
 
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
diff -r fe1050a60d8f -r c2b17bd27d7d tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Thu Sep 09 14:32:34 2010 +0100
+++ b/tools/libxl/libxltypes.py	Thu Sep 09 14:34:45 2010 +0100
@@ -33,6 +33,7 @@ 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 Number(Builtin):




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

Gianni Tedesco

2010-Sep-09 13:39 UTC

head link

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

On Thu, 2010-09-09 at 14:37 +0100, Ian Campbell wrote:> On Thu, 2010-09-09 at 14:07 +0100, Gianni Tedesco wrote:
> > 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.
> > 
> > Also 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.
> 
> You forgot to mention the addition of the Numbers type.
Ah yes
> Otherwise:
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > -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_file_reference = Builtin("file_reference",
> > +                               autogenerate_destructor=False,
> > +                              
destructor_fn="libxl_file_reference_destroy",
> > +                               passby=PASS_BY_REFERENCE)
> 
> autogenerate_destructor=False doesn''t do much on a Builtin in any
case
> since we never generate destructors for builtins. It should probably
> just be a default in the base type.
Oh right, fair enough.
> i.e. cumulatively on your patch:
> 
> Subject: libxl: Builtin types default to autogenerate_destructor=False
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r fe1050a60d8f -r c2b17bd27d7d tools/libxl/libxl.idl
> --- a/tools/libxl/libxl.idl	Thu Sep 09 14:32:34 2010 +0100
> +++ b/tools/libxl/libxl.idl	Thu Sep 09 14:34:45 2010 +0100
> @@ -14,6 +14,7 @@ libxl_nic_type = Number("nic_type", name
>  
>  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")
>  
> @@ -78,11 +79,6 @@ libxl_domain_create_info = Struct("domai
>      ("poolid",       uint32),
>      ("poolname",     string),
>      ])
> -
> -libxl_file_reference = Builtin("file_reference",
> -                               autogenerate_destructor=False,
> -                              
destructor_fn="libxl_file_reference_destroy",
> -                               passby=PASS_BY_REFERENCE)
>  
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
> diff -r fe1050a60d8f -r c2b17bd27d7d tools/libxl/libxltypes.py
> --- a/tools/libxl/libxltypes.py	Thu Sep 09 14:32:34 2010 +0100
> +++ b/tools/libxl/libxltypes.py	Thu Sep 09 14:34:45 2010 +0100
> @@ -33,6 +33,7 @@ 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 Number(Builtin):
> 
> 
> 


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

Gianni Tedesco

2010-Sep-09 14:38 UTC

head link

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

On Thu, 2010-09-09 at 14:07 +0100, Gianni Tedesco wrote:> diff -r b19856f6dd76 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c   Thu Sep 09 09:24:24 2010 +0100
> +++ b/tools/libxl/libxl_pci.c   Thu Sep 09 13:52:46 2010 +0100
> @@ -41,6 +41,15 @@
>  #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)
> +{
> +    return ((pcidev->reg & 0x3f) << 24) |
> +           ((pcidev->func & 0x7) << 21) |
> +           ((pcidev->dev & 0x1f) << 16) |
> +           ((pcidev->bus & 0xff) << 8) |
> +           ((pcidev->enable) ? 1 : 0);
> +}
> +
Gah, this bit is broken, don''t apply.

I assume it''s due to endianness - changed it to put it in a temporary
union and it works fine?!

Will re-send a better tested patch-series with more exported functions
next week.


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

Ian Jackson

2010-Sep-09 15:52 UTC

head link

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

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH]: change libxl IDL to export
a saner interface for upcoming language bindings"):> Will re-send a better tested patch-series with more exported functions
> next week.
OK.  Can you incorporate Ian Campbell''s change
  libxl: Builtin types default to autogenerate_destructor=False
either as a separate patch in your repost, or folded in ?

Ian.

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

Gianni Tedesco

2010-Sep-10 13:07 UTC

head link

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

On Thu, 2010-09-09 at 16:52 +0100, Ian Jackson wrote:> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH]: change libxl IDL to
export a saner interface for upcoming language bindings"):
> > Will re-send a better tested patch-series with more exported functions
> > next week.
> 
> OK.  Can you incorporate Ian Campbell''s change
>   libxl: Builtin types default to autogenerate_destructor=False
> either as a separate patch in your repost, or folded in ?
Ah yes, folded it in now


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

Xen devel - Sep 2010 - [PATCH]: change libxl IDL to export a saner interface for upcoming language bindings

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

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

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

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

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

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