Re: [Xen-devel] [PATCH v3-RESEND 09/28] libxl: make the libxl error type an IDL enum

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-11-04 at 14:48 +0000, Rob Hoes wrote:
> > On Mon, 2013-10-21 at 14:32 +0100, Rob Hoes wrote:
> > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > > index 16a92a4..11c53cf 100644
> > > --- a/tools/libxl/libxl_device.c
> > > +++ b/tools/libxl/libxl_device.c
> > > @@ -497,7 +497,7 @@ static void multidev_one_callback(libxl__egc *egc,
> > > libxl__ao_device *aodev)  {
> > >      STATE_AO_GC(aodev->ao);
> > >      libxl__multidev *multidev = aodev->multidev;
> > > -    int i, error = 0;
> > > +    int i, err = 0;
> > 
> > Why the spurious s/error/err/ ?
> > 
> > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > > index 5c43d6f..778a416 100644
> > > --- a/tools/libxl/libxl_types.idl
> > > +++ b/tools/libxl/libxl_types.idl
> > > @@ -28,6 +28,23 @@ MemKB = UInt(64, init_val =
> > "LIBXL_MEMKB_DEFAULT")
> > > # Constants / Enumerations  #
> > >
> > > +libxl_error = Enumeration("error", [
> > [...]
> > > +    ], namespace = "")
> > 
> > Ah, because you've defined "enum error" as an unnamespaced type.
> > Irrespective of the clash you've found I think this is a no go since an
> > application might reasonably be using "error" (also, err is a standard
> > function too, see err.h and/or err(3)).
> 
> Yes, I wanted to make sure that the enum value names did not change.
> With the default namespace, you'd get values such as
> "LIBXL_ERROR_FAIL" rather than "ERROR_FAIL".

OH, right, that makes sense.

> 
> > Assuming the idl doesn't support anonymous enums (I don't recall writing
> > anything to do that ;-)) and you quite reasonably don't want to add such
> > support I think enum libxl_error as the name is fine.
> 
> I think the IDL constructs enums as follows, for each value: namespace
> + "_" + enum name + "_" + value name. So naming the enum "libxl_error"
> again gives values such as "LIBXL_ERROR_FAIL", and we're back to
> square one :)
> 
> Perhaps we need another option in the IDL for this?

Yes, I think so. Something like a value_namespace option to the
containing enum would do the trick I think? Something like this
(untested, needs a corresponding idl.txt update) would allow you to pass
value_namespace = ""

diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
index 7d95e3f..0425384 100644
--- a/tools/libxl/idl.py
+++ b/tools/libxl/idl.py
@@ -136,7 +136,7 @@ class EnumerationValue(object):
 
         self.valuename = str.upper(name)
         self.rawname = str.upper(enum.rawname) + "_" + self.valuename
-        self.name = str.upper(enum.namespace) + self.rawname
+        self.name = str.upper(enum.value_namespace) + self.rawname
         self.value = value
 
 class Enumeration(Type):
@@ -144,6 +144,8 @@ class Enumeration(Type):
         kwargs.setdefault('dispose_fn', None)
         Type.__init__(self, typename, **kwargs)
 
+        self.value_namespace = kwargs.setdefault('value_namespace', 
self.namespace)
+
         self.values = []
         for v in values:
             # (value, name)

Ian.


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