[libvirt] libvirt boolean type
Hi. The following patch's intend is to get a discussion started first. Rationale: Many functions in libvirt return "1 for <TRUE>, 0 for <FALSE> and -1 on <ERROR>". When calling a function you need to read the documentation, skipping to the "Returns:" text in order to determine what return values to expect from the function and how to interpret them. This is cumbersome and takes an extra step which could be avoided. Advantages: + enum is binary compatible with int, so no ABI changes + without reading the documentation, you see what return values to expect at a glance + code generators in higher level languages could use it to generate proper wrappings to "boolean" automatically + compilers can probably warn when returning a value not part of the enum Claudio Bley (1): Implement a virBool type include/libvirt/libvirt.h.in | 25 +++++++++++++++++++++---- python/generator.py | 16 ++++++++++++++++ src/libvirt.c | 8 ++++---- 3 files changed, 41 insertions(+), 8 deletions(-) -- 1.7.9.5
Signed-off-by: Claudio Bley <cbley@av-test.de> --- include/libvirt/libvirt.h.in | 25 +++++++++++++++++++++---- python/generator.py | 16 ++++++++++++++++ src/libvirt.c | 8 ++++---- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c1233f6..b51f415 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -80,6 +80,23 @@ typedef void (*virFreeCallback)(void *opaque); /** + * virBool: + * + * A tri-state boolean type being able to signal an error in addition + * to having a value of true or false. + * + * 1 - true + * 0 - false + * -1 on error + */ +typedef enum { + VIR_BOOL_TRUE = 1, + VIR_BOOL_FALSE = 0, + VIR_BOOL_ERROR = -1 +} virBool; + + +/** * virConnect: * * a virConnect is a private structure representing a connection to @@ -3728,11 +3745,11 @@ int virNetworkIsPersistent(virNetworkPtr net); int virStoragePoolIsActive(virStoragePoolPtr pool); int virStoragePoolIsPersistent(virStoragePoolPtr pool); -int virInterfaceIsActive(virInterfacePtr iface); +virBool virInterfaceIsActive(virInterfacePtr iface); -int virConnectIsEncrypted(virConnectPtr conn); -int virConnectIsSecure(virConnectPtr conn); -int virConnectIsAlive(virConnectPtr conn); +virBool virConnectIsEncrypted(virConnectPtr conn); +virBool virConnectIsSecure(virConnectPtr conn); +virBool virConnectIsAlive(virConnectPtr conn); /* * CPU specification API diff --git a/python/generator.py b/python/generator.py index bae4edc..43df257 100755 --- a/python/generator.py +++ b/python/generator.py @@ -266,6 +266,8 @@ py_types = { 'const char *': ('z', None, "constcharPtr", "const char *"), 'size_t': ('n', None, "size_t", "size_t"), + 'virBool': ('i', None, "int", "int"), + 'virDomainPtr': ('O', "virDomain", "virDomainPtr", "virDomainPtr"), 'const virDomainPtr': ('O', "virDomain", "virDomainPtr", "virDomainPtr"), 'virDomain *': ('O', "virDomain", "virDomainPtr", "virDomainPtr"), @@ -1291,6 +1293,13 @@ def buildWrappers(module): classes.write(classes_type[ret[0]][1] % ("ret")); classes.write("\n"); + # special case for virBool + elif ret[0] == 'virBool': + classes.write((" if " + functions_int_default_test + + ": raise libvirtError ('%s() failed')\n" + + " return (ret == 1)\n") % + ("ret", name)) + # For functions returning an integral type there are # several things that we can do, depending on the # contents of functions_int_*: @@ -1529,6 +1538,13 @@ def buildWrappers(module): classes.write(converter_type[ret[0]] % ("ret")); classes.write("\n"); + # special case for virBool + elif ret[0] == 'virBool': + classes.write((" if " + functions_int_default_test + + ": raise libvirtError ('%s() failed')\n" + + " return (ret == 1)\n") % + ("ret", name)) + # For functions returning an integral type there # are several things that we can do, depending on # the contents of functions_int_*: diff --git a/src/libvirt.c b/src/libvirt.c index a783fa6..2e298d7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17112,7 +17112,7 @@ virNWFilterRef(virNWFilterPtr nwfilter) * * Returns 1 if running, 0 if inactive, -1 on error */ -int virInterfaceIsActive(virInterfacePtr iface) +virBool virInterfaceIsActive(virInterfacePtr iface) { VIR_DEBUG("iface=%p", iface); @@ -17146,7 +17146,7 @@ error: * * Returns 1 if encrypted, 0 if not encrypted, -1 on error */ -int virConnectIsEncrypted(virConnectPtr conn) +virBool virConnectIsEncrypted(virConnectPtr conn) { VIR_DEBUG("conn=%p", conn); @@ -17183,7 +17183,7 @@ error: * * Returns 1 if secure, 0 if secure, -1 on error */ -int virConnectIsSecure(virConnectPtr conn) +virBool virConnectIsSecure(virConnectPtr conn) { VIR_DEBUG("conn=%p", conn); @@ -19858,7 +19858,7 @@ error: * * Returns 1 if alive, 0 if dead, -1 on error */ -int virConnectIsAlive(virConnectPtr conn) +virBool virConnectIsAlive(virConnectPtr conn) { VIR_DEBUG("conn=%p", conn); -- 1.7.9.5
On Fri, Jan 11, 2013 at 04:40:31PM +0100, Claudio Bley wrote:
Hi.
The following patch's intend is to get a discussion started first.
Rationale:
Many functions in libvirt return "1 for <TRUE>, 0 for <FALSE> and -1 on <ERROR>".
When calling a function you need to read the documentation, skipping to the "Returns:" text in order to determine what return values to expect from the function and how to interpret them.
This is cumbersome and takes an extra step which could be avoided.
Advantages:
+ enum is binary compatible with int, so no ABI changes
I'm afraid that is wrong. Size of enums is undefined in C. The compiler is free to use any integer size to represent an enum. As such we have a rule that no enums are allowed in libvirt API signatures. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
At Fri, 11 Jan 2013 16:11:35 +0000, Daniel P. Berrange wrote: [Daniel, I meant to send my reply to the list on Friday. Maybe it got stuck somewhere or I had only sent it to you alone - I was using a different MUA using a Live-CD... Sorry for the duplicate, if any.]
On Fri, Jan 11, 2013 at 04:40:31PM +0100, Claudio Bley wrote:
Hi.
The following patch's intend is to get a discussion started first.
Rationale:
Many functions in libvirt return "1 for <TRUE>, 0 for <FALSE> and -1 on <ERROR>".
When calling a function you need to read the documentation, skipping to the "Returns:" text in order to determine what return values to expect from the function and how to interpret them.
This is cumbersome and takes an extra step which could be avoided.
Advantages:
+ enum is binary compatible with int, so no ABI changes
I'm afraid that is wrong. Size of enums is undefined in C. The compiler is free to use any integer size to represent an enum. As such we have a rule that no enums are allowed in libvirt API signatures.
That's too bad. This surely diminishes the value of this addition (a bit). Nonetheless, I think it still would be valuable as point 2 and 3 still hold. Just change the definition to: typedef int virBool; Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On 01/14/2013 01:02 AM, Claudio Bley wrote:
Nonetheless, I think it still would be valuable as point 2 and 3 still hold. Just change the definition to:
typedef int virBool;
I'm not too fond of using the term 'bool' for anything tri-state - to me, bool implies exactly two states. _Maybe_ you could get away with a typedef for a different name (virTristate?), but at some point, 'int' is so much easier to type than whatever new typedef, that I don't think we would be buying much with this proposal. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jan 15, 2013 at 03:34:42PM -0700, Eric Blake wrote:
On 01/14/2013 01:02 AM, Claudio Bley wrote:
Nonetheless, I think it still would be valuable as point 2 and 3 still hold. Just change the definition to:
typedef int virBool;
I'm not too fond of using the term 'bool' for anything tri-state - to me, bool implies exactly two states. _Maybe_ you could get away with a typedef for a different name (virTristate?), but at some point, 'int' is so much easier to type than whatever new typedef, that I don't think we would be buying much with this proposal.
Yeah, I really just prefer the code as it is now. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Claudio Bley -
Daniel P. Berrange -
Eric Blake