Re: [PATCH xm/xl enhancements for vptm 5/6] make devid a libxl type

[Matthew Fioravante <matthew.fioravante@jhuapl.edu>]

[-- Attachment #1.1: Type: text/plain, Size: 9590 bytes --]

On 09/25/2012 12:14 PM, Ian Campbell wrote:
> On Tue, 2012-09-25 at 17:00 +0100, Matthew Fioravante wrote:
>> On 09/25/2012 06:36 AM, Ian Campbell wrote:
>>> On Fri, 2012-09-21 at 20:20 +0100, Matthew Fioravante wrote:
>>>> This fixes a bug in libxl where device ids are not initialized properly.
>>>> The devid field for each device is set to be an integer type which are
>>>> always initialized to 0.
>>>>
>>>> This makes the xl DEV-attach commands always fail to add more than one
>>>> device, because each time the device id is initialized to 0, when it
>>>> should be initialized to -1, which in the code will trigger a search for
>>>> free id.
>>> Which of the DEV-attach commands can be used to add more than one
>>> device?
>> network-attach, block-attach, and also my vtpm-attach.
> Oh, you mean by being called repeatedly, not adding multiple devices in
> one go!
>
>>> Where is the code to do the search? I had a look in the disk and network
>>> cases and couldn't find it.
>> libxl.c
>>
>> void libxl__device_nic_add(
>>
>>     if (nic->devid == -1) {
>>         if (!(dompath = libxl__xs_get_dompath(gc, domid))) {
>>             rc = ERROR_FAIL;
>>             goto out_free;
>>         }
>>         if (!(l = libxl__xs_directory(gc, XBT_NULL,
>>                                      libxl__sprintf(gc, "%s/device/vif",
>> dompath), &nb))) {
>>             nic->devid = 0;
>>         } else {
>>             nic->devid = strtoul(l[nb - 1], NULL, 10) + 1;
>>         }
>>     }
> Not sure how I missed this.
>
>> Right now, nic-devid is 0 on attach.
> Is devid not a parameter to network-attach? It is to block-attach.
It might be, but if its not specified I think the logical thing to do is
to automatically choose the next available id.
>
>> So it always tries to use 0. When you do a network-attach twice,
>> the second time it trys and fails to create device/vif/0
> Your change to use -1 as thye default is certainly correct. I'm just
> trying to figure out why xl does things this way in the first place.
>
>>>> diff --git a/tools/ocaml/libs/xs/xs.mli b/tools/ocaml/libs/xs/xs.mli
>>>> --- a/tools/ocaml/libs/xs/xs.mli
>>>> +++ b/tools/ocaml/libs/xs/xs.mli
>>>> @@ -27,6 +27,7 @@ exception Failed_to_connect
>>>>  type perms = Xsraw.perms
>>>>  
>>>>  type domid = int
>>>> +type devid = int
>>> I can see why these were needed in the xl modules, but I don't see how
>>> this type can be required in the xenstore ones.
>> It shouldn't be required for xenstore or xc. The problem is they won't
>> compile without it because of the way the ocaml build system works. As
>> far as I understand it creates types for xl, xc, and xs from the
>> libxl_types.idl.
> No, only xl does this.
>
> The others are the libxc and xenstore bindings which are completely hand
> coded and there is no concept of a devid at those layers anyway. What
> error do you get if you omit the changes to those module?
Ok I must be crazy. I went back and removed them and tried to compile it
again and it works fine. I think I was forgetting to update both the
.mli and the .ml files. Anyway a new patch with the xc/xs removed is
attached.
>
>>  Either the build system needs to be changed, or devid
>> needs to be a type in all libraries, or we find some other way to fix
>> this initialization bug.
>>> Ian.
>>>
>>
>

Signed off by Matthew Fioravante matthew.fioravante@jhuapl.edu

---
* Rebased off of latest xen-unstable
* Fixed whitespace errors
* Removed devid from libxc and libxs

diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -60,7 +60,7 @@ def gen_rand_init(ty, v, indent = "    ", parent = None):
                                         passby=idl.PASS_BY_REFERENCE))
     elif ty.typename in ["libxl_uuid", "libxl_mac", "libxl_hwcap"]:
         s += "rand_bytes((uint8_t *)%s, sizeof(*%s));\n" % (v,v)
-    elif ty.typename in ["libxl_domid"] or isinstance(ty, idl.Number):
+    elif ty.typename in ["libxl_domid", "libxl_devid"] or isinstance(ty, idl.Number):
         s += "%s = rand() %% (sizeof(%s)*8);\n" % \
              (ty.pass_arg(v, parent is None),
               ty.pass_arg(v, parent is None))
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -307,6 +307,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *cpuid_list);
 #define LIBXL_PCI_FUNC_ALL (~0U)
 
 typedef uint32_t libxl_domid;
+typedef int libxl_devid;
 
 /*
  * Formatting Enumerations.
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -8,6 +8,7 @@ namespace("libxl_")
 libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
 
 libxl_domid = Builtin("domid", json_fn = "yajl_gen_integer", autogenerate_json = False)
+libxl_devid = Builtin("devid", json_fn = "yajl_gen_integer", autogenerate_json = False, signed = True, init_val="-1")
 libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
 libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
 libxl_bitmap = Builtin("bitmap", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
@@ -343,7 +344,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
 libxl_device_vfb = Struct("device_vfb", [
     ("backend_domid", libxl_domid),
-    ("devid",         integer),
+    ("devid",         libxl_devid),
     ("vnc",           libxl_vnc_info),
     ("sdl",           libxl_sdl_info),
     # set keyboard layout, default is en-us keyboard
@@ -352,7 +353,7 @@ libxl_device_vfb = Struct("device_vfb", [
 
 libxl_device_vkb = Struct("device_vkb", [
     ("backend_domid", libxl_domid),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ])
 
 libxl_device_disk = Struct("device_disk", [
@@ -369,7 +370,7 @@ libxl_device_disk = Struct("device_disk", [
 
 libxl_device_nic = Struct("device_nic", [
     ("backend_domid", libxl_domid),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ("mtu", integer),
     ("model", string),
     ("mac", libxl_mac),
@@ -399,7 +400,7 @@ libxl_diskinfo = Struct("diskinfo", [
     ("backend_id", uint32),
     ("frontend", string),
     ("frontend_id", uint32),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ("state", integer),
     ("evtch", integer),
     ("rref", integer),
@@ -410,7 +411,7 @@ libxl_nicinfo = Struct("nicinfo", [
     ("backend_id", uint32),
     ("frontend", string),
     ("frontend_id", uint32),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ("state", integer),
     ("evtch", integer),
     ("rref_tx", integer),
diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -10,6 +10,7 @@ builtins = {
     "int":                  ("int",                    "%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
     "char *":               ("string",                 "%(c)s = dup_String_val(gc, %(o)s)", "caml_copy_string(%(c)s)"),
     "libxl_domid":          ("domid",                  "%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
+    "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),
@@ -41,8 +42,8 @@ def stub_fn_name(ty, name):
     return "stub_xl_%s_%s" % (ty.rawname,name)
     
 def ocaml_type_of(ty):
-    if ty.rawname == "domid":
-        return "domid"
+    if ty.rawname in ["domid","devid"]:
+        return ty.rawname
     elif isinstance(ty,idl.UInt):
         if ty.width in [8, 16]:
             # handle as ints
diff --git a/tools/ocaml/libs/xl/xenlight.ml.in b/tools/ocaml/libs/xl/xenlight.ml.in
--- a/tools/ocaml/libs/xl/xenlight.ml.in
+++ b/tools/ocaml/libs/xl/xenlight.ml.in
@@ -16,6 +16,7 @@
 exception Error of string
 
 type domid = int
+type devid = int
 
 (* @@LIBXL_TYPES@@ *)
 
diff --git a/tools/ocaml/libs/xl/xenlight.mli.in b/tools/ocaml/libs/xl/xenlight.mli.in
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -16,6 +16,7 @@
 exception Error of string
 
 type domid = int
+type devid = int
 
 (* @@LIBXL_TYPES@@ *)
 
diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -281,6 +281,11 @@ int attrib__libxl_domid_set(PyObject *v, libxl_domid *domid) {
     return 0;
 }
 
+int attrib__libxl_devid_set(PyObject *v, libxl_devid *devid) {
+   *devid = PyInt_AsLong(v);
+   return 0;
+}
+
 int attrib__struct_in_addr_set(PyObject *v, struct in_addr *pptr)
 {
     PyErr_SetString(PyExc_NotImplementedError, "Setting in_addr");
@@ -342,6 +347,10 @@ PyObject *attrib__libxl_domid_get(libxl_domid *domid) {
     return PyInt_FromLong(*domid);
 }
 
+PyObject *attrib__libxl_devid_get(libxl_devid *devid) {
+    return PyInt_FromLong(*devid);
+}
+
 PyObject *attrib__struct_in_addr_get(struct in_addr *pptr)
 {
     PyErr_SetString(PyExc_NotImplementedError, "Getting in_addr");
-- 
1.7.4.4





[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 1459 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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