ocaml libxl bindings and KeyedUnion

ocaml libxl bindings and KeyedUnion

[Olaf Hering]

Some change commited to staging earlier this week breaks ocaml bindings,
as shown below. I think it was introduced between
a68d1b65bb1bbb9b8db2d82695d32ac09c52a2d7..d4ea77c9d7b314001ff4e2eb7618b45f11551371:

NotImplementedError: Cannot handle KeyedUnion fields which are not Structs

But now that I look at it, may it be my pvscsi change by any chance?
https://github.com/olafhering/xen/commit/f0de53755f628efe64cd60d4511a9667485eba62

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 48cd9af..2bd050d 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -556,12 +556,21 @@ libxl_vscsi_hctl = Struct("vscsi_hctl", [
     ("lun", uint32),
     ])
 
+libxl_vscsi_pdev = Struct("vscsi_pdev", [
+    ("p_devname",        string),
+    ("u", KeyedUnion(None, libxl_vscsi_pdev_type, "type",
+        [
+         ("invalid", None),
+         ("dev", libxl_vscsi_hctl),
+         ("wwn", string),
+         ("hctl", libxl_vscsi_hctl),
+        ])),
+    ])
+
 libxl_vscsi_dev = Struct("vscsi_dev", [
     ("vscsi_dev_id",     libxl_devid),
     ("remove",           bool),
-    ("p_devname",        string),
-    ("pdev_type",        libxl_vscsi_pdev_type),
-    ("pdev",             libxl_vscsi_hctl),
+    ("pdev",             libxl_vscsi_pdev),
     ("vdev",             libxl_vscsi_hctl),
     ])
 
@@ -624,8 +633,7 @@ libxl_vscsiinfo = Struct("vscsiinfo", [
     ("frontend", string),
     ("frontend_id", uint32),
     ("devid", libxl_devid),
-    ("p_devname", string),
-    ("pdev", libxl_vscsi_hctl),
+    ("pdev", libxl_vscsi_pdev),
     ("vdev", libxl_vscsi_hctl),
     ("vscsi_dev_id", libxl_devid),
     ("feature_host", bool),

Olaf

....
ocamlc -g -I ../xb/ -w F -warn-error F -c -o xsraw.cmi xsraw.mli
ocamlc -g -I ../xb/ -w F -warn-error F -c -o xsraw.cmo xsraw.ml
ocamlc -g -I ../xb/ -w F -warn-error F -c -o xst.cmi xst.mli
ocamlc -g -I ../xb/ -w F -warn-error F -c -o xst.cmo xst.ml
ocamlc -g -I ../xb/ -w F -warn-error F -c -o xs.cmi xs.mli
ocamlc -g -I ../xb/ -w F -warn-error F -c -o xs.cmo xs.ml
ocamlc -pack -o xenstore.cmo queueop.cmo xsraw.cmo xst.cmo xs.cmo
ocamlc -g -I ../xb/ -w F -warn-error F -a -o xenstore.cma    xenstore.cmo
ocamlopt -g -ccopt "   -Wl,-rpath,/opt/xen/upstream/staging-wip/lib64" -dtypes -I ../xb/ -cc gcc -w F -warn-error F -for-pack Xenstore -c -o queueop.cmx queueop.ml
ocamlopt -g -ccopt "   -Wl,-rpath,/opt/xen/upstream/staging-wip/lib64" -dtypes -I ../xb/ -cc gcc -w F -warn-error F -for-pack Xenstore -c -o xsraw.cmx xsraw.ml
ocamlopt -g -ccopt "   -Wl,-rpath,/opt/xen/upstream/staging-wip/lib64" -dtypes -I ../xb/ -cc gcc -w F -warn-error F -for-pack Xenstore -c -o xst.cmx xst.ml
ocamlopt -g -ccopt "   -Wl,-rpath,/opt/xen/upstream/staging-wip/lib64" -dtypes -I ../xb/ -cc gcc -w F -warn-error F -for-pack Xenstore -c -o xs.cmx xs.ml
ocamlopt -pack -o xenstore.cmx queueop.cmx xsraw.cmx xst.cmx xs.cmx
ocamlopt -g -ccopt "   -Wl,-rpath,/opt/xen/upstream/staging-wip/lib64" -dtypes -I ../xb/ -cc gcc -w F -warn-error F -for-pack Xenstore -a -o xenstore.cmxa    xenstore.cmx
sed 's/@VERSION@/4.1/g' < META.in >META.new && mv -f META.new META
mkdir -p /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml
ocamlfind remove -destdir /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml xenstore
ocamlfind: [WARNING] No such directory: /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore
ocamlfind install -destdir /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml -ldconf ignore xenstore META xenstore.cma xenstore.cmxa xenstore.cmo xenstore.cmi xenstore.cmx *.a
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/xenstore.a
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/xenstore.cmx
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/xenstore.cmi
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/xenstore.cmo
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/xenstore.cmxa
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/xenstore.cma
Installed /work/olaf/13.1/github/olafhering/xen.git/dist/install//opt/xen/upstream/staging-wip/lib64/ocaml/xenstore/META
make[7]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs/xs'
make[6]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs'
make[6]: Entering directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs'
make -C xl install
make[7]: Entering directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs/xl'
PYTHONPATH=/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs/xl/../../../../tools/libxl python genwrap.py \
        /work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs/xl/../../../../tools/libxl/libxl_types.idl \
        _libxl_types.mli.in _libxl_types.ml.in _libxl_types.inc
Parsing /work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs/xl/../../../../tools/libxl/libxl_types.idl
Traceback (most recent call last):
  File "genwrap.py", line 529, in <module>
    ml.write(gen_ocaml_ml(ty, False))
  File "genwrap.py", line 208, in gen_ocaml_ml
    ku, union_type = gen_ocaml_keyedunions(f.type, interface, "\t")
  File "genwrap.py", line 161, in gen_ocaml_keyedunions
    raise NotImplementedError("Cannot handle KeyedUnion fields which are not Structs")
NotImplementedError: Cannot handle KeyedUnion fields which are not Structs
make[7]: *** No rule to make target `_libxl_types.ml.in', needed by `xenlight.ml'.  Stop.
make[7]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs/xl'
make[6]: *** [subdir-install-xl] Error 2
make[6]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs'
make[5]: *** [subdirs-install] Error 2
make[5]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml/libs'
make[4]: *** [subdir-install-libs] Error 2
make[4]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml'
make[3]: *** [subdirs-install] Error 2
make[3]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools/ocaml'
make[2]: *** [subdir-install-ocaml] Error 2
make[2]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools'
make[1]: *** [subdirs-install] Error 2
make[1]: Leaving directory `/work/olaf/13.1/github/olafhering/xen.git/tools'
make: *** [install-tools] Error 2

real    4m44.888s
user    11m19.208s
sys     1m50.444s
root@optiplex:/work/olaf/13.1/github/olafhering/xen.git #

Re: ocaml libxl bindings and KeyedUnion

[Ian Campbell]

On Fri, 2015-03-20 at 09:10 +0100, Olaf Hering wrote:
> Some change commited to staging earlier this week breaks ocaml bindings,
> as shown below. I think it was introduced between
> a68d1b65bb1bbb9b8db2d82695d32ac09c52a2d7..d4ea77c9d7b314001ff4e2eb7618b45f11551371:
> 
> NotImplementedError: Cannot handle KeyedUnion fields which are not Structs

I don't see anything in that range which I would expect to cause
anything like this so...

> But now that I look at it, may it be my pvscsi change by any chance?
> https://github.com/olafhering/xen/commit/f0de53755f628efe64cd60d4511a9667485eba62

... yes, probably. Although surely it would have been easier for you to
confirm this yourself than asking everyone to speculate, especially if
you are going to start your mail blaming something in staging.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 48cd9af..2bd050d 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -556,12 +556,21 @@ libxl_vscsi_hctl = Struct("vscsi_hctl", [
>      ("lun", uint32),
>      ])
>  
> +libxl_vscsi_pdev = Struct("vscsi_pdev", [
> +    ("p_devname",        string),
> +    ("u", KeyedUnion(None, libxl_vscsi_pdev_type, "type",
> +        [
> +         ("invalid", None),
> +         ("dev", libxl_vscsi_hctl),
> +         ("wwn", string),
> +         ("hctl", libxl_vscsi_hctl),

Following the other examples in libxl_types.idl this would need to be
something like:
         ("invalid", None),
         ("dev", Struct(None, [(libxl_vscsi_hctl, "dev")])),
         ("wwn", Struct(None, [(string, "wwn")])),
         ("hctl", Struct(None, [(libxl_vscsi_hctl, "hctl")])),
           ^(*)                                     ^(**)

Or you could teach the IDL machinery and code generators about
non-struct members of keyed unions but a) that will be a lot of faff and
b) it will make it hard to extend any one of the members in the future.

The name at (*) must be the enum member, which I've duplicated at (**)
but you might like to thing about whether (**) would have a better name
in the context of e.g. vscsi_dev->u.dev.dev or vscsi_dev->u.wwn.wwn.

Aside: What is the difference between dev and hctl in this context?

Ian.

Re: ocaml libxl bindings and KeyedUnion

[Olaf Hering]

On Fri, Mar 20, Ian Campbell wrote:

> The name at (*) must be the enum member, which I've duplicated at (**)
> but you might like to thing about whether (**) would have a better name
> in the context of e.g. vscsi_dev->u.dev.dev or vscsi_dev->u.wwn.wwn.

Thanks Ian. For some reason I have ocaml disabled on my workstation.
I made this change:

+++ b/tools/libxl/libxl_types.idl
@@ -571,9 +571,9 @@ libxl_vscsi_pdev = Struct("vscsi_pdev", [
     ("u", KeyedUnion(None, libxl_vscsi_pdev_type, "type",
         [
          ("invalid", None),
-         ("dev", libxl_vscsi_hctl),
-         ("wwn", string),
-         ("hctl", libxl_vscsi_hctl),
+         ("dev", Struct(None, [("m", libxl_vscsi_hctl)])),
+         ("wwn", Struct(None, [("m", string)])),
+         ("hctl", Struct(None, [("m", libxl_vscsi_hctl)])),
         ])),
     ])

> Aside: What is the difference between dev and hctl in this context?

Its supposed to represent either "/dev/something" and "h:ct:l". The
result in the "p-dev" property, which is used by the backend, is the
same.  But translating "p-dev" back into the config string for the
scsi-list command needs some way to represent that. I'm not fully happy
with the current way. Perhaps the code should just reuse the "p-devname"
property to tell what was in the config file.

Olaf

Re: ocaml libxl bindings and KeyedUnion

[Ian Campbell]

On Fri, 2015-03-20 at 17:44 +0100, Olaf Hering wrote:
> +         ("dev", Struct(None, [("m", libxl_vscsi_hctl)])),
> +         ("wwn", Struct(None, [("m", string)])),
> +         ("hctl", Struct(None, [("m", libxl_vscsi_hctl)])),

> > Aside: What is the difference between dev and hctl in this context?
> 
> Its supposed to represent either "/dev/something" and "h:ct:l".

So shouldn't dev by a string then?

>  The
> result in the "p-dev" property, which is used by the backend, is the
> same.  But translating "p-dev" back into the config string for the
> scsi-list command needs some way to represent that. I'm not fully happy
> with the current way. Perhaps the code should just reuse the "p-devname"
> property to tell what was in the config file.

Perhaps the list command should just list the canonical name for the
device (i.e p-dev? a h:c:t:l tuple) and not worry about matching the
config file?

I'd even go as far as suggesting that the libxl API ought only to deal
with the canonical name and that the toolstack can parse whatever forms
it likes into that (perhaps using a helper from libxlu). That would make
the libxl interface a lot simpler, wouldn't it?

Ian.

Re: ocaml libxl bindings and KeyedUnion

[Olaf Hering]

On Fri, Mar 20, Ian Campbell wrote:

> On Fri, 2015-03-20 at 17:44 +0100, Olaf Hering wrote:
> > +         ("dev", Struct(None, [("m", libxl_vscsi_hctl)])),
> > +         ("wwn", Struct(None, [("m", string)])),
> > +         ("hctl", Struct(None, [("m", libxl_vscsi_hctl)])),
> 
> > > Aside: What is the difference between dev and hctl in this context?
> > 
> > Its supposed to represent either "/dev/something" and "h:ct:l".
> 
> So shouldn't dev by a string then?

No, the result will be written to the "p-dev" property.

> >  The
> > result in the "p-dev" property, which is used by the backend, is the
> > same.  But translating "p-dev" back into the config string for the
> > scsi-list command needs some way to represent that. I'm not fully happy
> > with the current way. Perhaps the code should just reuse the "p-devname"
> > property to tell what was in the config file.
> 
> Perhaps the list command should just list the canonical name for the
> device (i.e p-dev? a h:c:t:l tuple) and not worry about matching the
> config file?

I will think about this suggestion. Right now the scsilist command does
just what xm did.

> I'd even go as far as suggesting that the libxl API ought only to deal
> with the canonical name and that the toolstack can parse whatever forms
> it likes into that (perhaps using a helper from libxlu). That would make
> the libxl interface a lot simpler, wouldn't it?

Olaf