[libvirt] [python PATCH] build: make it easier to backport event ids
In some cases, it is very easy for downstream distros to backport enum values without requiring a .so bump. Keying the conditional code off of the upstream version where the enum value was added is not ideal, because downstream then has to patch that the feature is available in their build that still reports an earlier version number. For example, if RHEL 7 backports events from 1.2.11 into a build based on 1.2.8, building the python bindings would warn: libvirt-override.c: In function ‘libvirt_virConnectDomainEventRegisterAny’: libvirt-override.c:6653:5: warning: enumeration value ‘VIR_DOMAIN_EVENT_ID_TUNABLE’ not handled in switch [-Wswitch] switch ((virDomainEventID) eventID) { ^ libvirt-override.c:6653:5: warning: enumeration value ‘VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE’ not handled in switch [-Wswitch] The solution is simple - use feature-based probes instead of version probes. Since we already scrape the XML API document of whatever libvirt build we are binding, and that XML already documents any downstream enum additions, we can use those as the features for gating conditional compilation. * generator.py (enum): Track event id names. (buildStubs): Output define wrappers for events. * libvirt-override.c (libvirt_virConnectDomainEventBalloonChangeCallback) (libvirt_virConnectDomainEventPMSuspendDiskCallback) (libvirt_virConnectDomainEventDeviceRemovedCallback) (libvirt_virConnectDomainEventTunableCallback) (libvirt_virConnectDomainEventAgentLifecycleCallback) (libvirt_virConnectDomainEventRegisterAny): Use them. Signed-off-by: Eric Blake <eblake@redhat.com> --- I mentioned this idea on the list a while back, and finally got time to code it up in a much nicer way than my initial attempt that would have polluted libvirt.h proper. https://www.redhat.com/archives/libvir-list/2014-October/msg00186.html I'm relatively weak at python, and welcome to suggestions on how to avoid the long line in generator.py (maybe a regex match on VIR_.*_EVENT_ID_, but what's the canonical way to write that in python?) generator.py | 11 +++++++++-- libvirt-override.c | 46 +++++++++++++++++++++++----------------------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/generator.py b/generator.py index cf044c9..4fd6d55 100755 --- a/generator.py +++ b/generator.py @@ -9,6 +9,7 @@ qemu_functions = {} enums = {} # { enumType: { enumConstant: enumValue } } lxc_enums = {} # { enumType: { enumConstant: enumValue } } qemu_enums = {} # { enumType: { enumConstant: enumValue } } +event_ids = [] import os import sys @@ -219,6 +220,8 @@ def lxc_function(name, desc, ret, args, file, module, cond): def enum(type, name, value): if type not in enums: enums[type] = {} + if name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_'): + event_ids.append(name) if value == 'VIR_TYPED_PARAM_INT': value = 1 elif value == 'VIR_TYPED_PARAM_UINT': @@ -910,10 +913,10 @@ def buildStubs(module, api_xml): wrapper_file = "build/%s.c" % module include = open(header_file, "w") - include.write("/* Generated */\n\n") + include.write("/* Generated by generator.py */\n\n") export = open(export_file, "w") - export.write("/* Generated */\n\n") + export.write("/* Generated by generator.py */\n\n") wrapper = open(wrapper_file, "w") wrapper.write("/* Generated by generator.py */\n\n") @@ -943,6 +946,10 @@ def buildStubs(module, api_xml): # Write C pointer conversion functions. for classname in primary_classes: print_c_pointer(classname, wrapper, export, include) + # Write define wrappers around event id enums + include.write("\n") + for event_id in event_ids: + include.write("#define %s %s\n" % (event_id, event_id)) include.close() export.close() diff --git a/libvirt-override.c b/libvirt-override.c index e51c44d..88cb527 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -4,7 +4,7 @@ * entry points where an automatically generated stub is * unpractical * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * Daniel Veillard <veillard@redhat.com> */ @@ -6346,7 +6346,7 @@ libvirt_virConnectDomainEventPMSuspendCallback(virConnectPtr conn ATTRIBUTE_UNUS } -#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE static int libvirt_virConnectDomainEventBalloonChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6398,9 +6398,9 @@ libvirt_virConnectDomainEventBalloonChangeCallback(virConnectPtr conn ATTRIBUTE_ LIBVIRT_RELEASE_THREAD_STATE; return ret; } -#endif /* LIBVIR_CHECK_VERSION(0, 10, 0) */ +#endif /* VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE */ -#if LIBVIR_CHECK_VERSION(1, 0, 0) +#ifdef VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK static int libvirt_virConnectDomainEventPMSuspendDiskCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6452,9 +6452,9 @@ libvirt_virConnectDomainEventPMSuspendDiskCallback(virConnectPtr conn ATTRIBUTE_ LIBVIRT_RELEASE_THREAD_STATE; return ret; } -#endif /* LIBVIR_CHECK_VERSION(1, 0, 0) */ +#endif /* VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK */ -#if LIBVIR_CHECK_VERSION(1, 1, 1) +#ifdef VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED static int libvirt_virConnectDomainEventDeviceRemovedCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6504,9 +6504,9 @@ libvirt_virConnectDomainEventDeviceRemovedCallback(virConnectPtr conn ATTRIBUTE_ LIBVIRT_RELEASE_THREAD_STATE; return ret; } -#endif /* LIBVIR_CHECK_VERSION(1, 1, 1) */ +#endif /* VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED */ -#if LIBVIR_CHECK_VERSION(1, 2, 9) +#ifdef VIR_DOMAIN_EVENT_ID_TUNABLE static int libvirt_virConnectDomainEventTunableCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6564,9 +6564,9 @@ libvirt_virConnectDomainEventTunableCallback(virConnectPtr conn ATTRIBUTE_UNUSED return ret; } -#endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */ +#endif /* VIR_DOMAIN_EVENT_ID_TUNABLE */ -#if LIBVIR_CHECK_VERSION(1, 2, 11) +#ifdef VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE static int libvirt_virConnectDomainEventAgentLifecycleCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6618,7 +6618,7 @@ libvirt_virConnectDomainEventAgentLifecycleCallback(virConnectPtr conn ATTRIBUTE return ret; } -#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ +#endif /* VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE */ static PyObject * @@ -6676,9 +6676,9 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject *self, cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventGenericCallback); break; case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: -#if LIBVIR_CHECK_VERSION(1, 2, 6) +#ifdef VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: -#endif /* LIBVIR_CHECK_VERSION(1, 2, 6) */ +#endif /* VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 */ cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventBlockJobCallback); break; case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: @@ -6693,31 +6693,31 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject *self, case VIR_DOMAIN_EVENT_ID_PMSUSPEND: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventPMSuspendCallback); break; -#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventBalloonChangeCallback); break; -#endif /* LIBVIR_CHECK_VERSION(0, 10, 0) */ -#if LIBVIR_CHECK_VERSION(1, 0, 0) +#endif /* VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE */ +#ifdef VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventPMSuspendDiskCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 0, 0) */ -#if LIBVIR_CHECK_VERSION(1, 1, 1) +#endif /* VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK */ +#ifdef VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventDeviceRemovedCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 1, 1) */ -#if LIBVIR_CHECK_VERSION(1, 2, 9) +#endif /* VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED */ +#ifdef VIR_DOMAIN_EVENT_ID_TUNABLE case VIR_DOMAIN_EVENT_ID_TUNABLE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventTunableCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */ -#if LIBVIR_CHECK_VERSION(1, 2, 11) +#endif /* VIR_DOMAIN_EVENT_ID_TUNABLE */ +#ifdef VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventAgentLifecycleCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ +#endif /* VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE */ case VIR_DOMAIN_EVENT_ID_LAST: break; } -- 2.1.0
On Wed, Jan 14, 2015 at 08:08:16PM -0700, Eric Blake wrote:
In some cases, it is very easy for downstream distros to backport enum values without requiring a .so bump. Keying the conditional code off of the upstream version where the enum value was added is not ideal, because downstream then has to patch that the feature is available in their build that still reports an earlier version number. For example, if RHEL 7 backports events from 1.2.11 into a build based on 1.2.8, building the python bindings would warn:
libvirt-override.c: In function ‘libvirt_virConnectDomainEventRegisterAny’: libvirt-override.c:6653:5: warning: enumeration value ‘VIR_DOMAIN_EVENT_ID_TUNABLE’ not handled in switch [-Wswitch] switch ((virDomainEventID) eventID) { ^ libvirt-override.c:6653:5: warning: enumeration value ‘VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE’ not handled in switch [-Wswitch]
The solution is simple - use feature-based probes instead of version probes. Since we already scrape the XML API document of whatever libvirt build we are binding, and that XML already documents any downstream enum additions, we can use those as the features for gating conditional compilation.
* generator.py (enum): Track event id names. (buildStubs): Output define wrappers for events. * libvirt-override.c (libvirt_virConnectDomainEventBalloonChangeCallback) (libvirt_virConnectDomainEventPMSuspendDiskCallback) (libvirt_virConnectDomainEventDeviceRemovedCallback) (libvirt_virConnectDomainEventTunableCallback) (libvirt_virConnectDomainEventAgentLifecycleCallback) (libvirt_virConnectDomainEventRegisterAny): Use them.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
I mentioned this idea on the list a while back, and finally got time to code it up in a much nicer way than my initial attempt that would have polluted libvirt.h proper. https://www.redhat.com/archives/libvir-list/2014-October/msg00186.html
I'm relatively weak at python, and welcome to suggestions on how to avoid the long line in generator.py (maybe a regex match on VIR_.*_EVENT_ID_, but what's the canonical way to write that in python?)
generator.py | 11 +++++++++-- libvirt-override.c | 46 +++++++++++++++++++++++----------------------- 2 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/generator.py b/generator.py index cf044c9..4fd6d55 100755 --- a/generator.py +++ b/generator.py @@ -9,6 +9,7 @@ qemu_functions = {} enums = {} # { enumType: { enumConstant: enumValue } } lxc_enums = {} # { enumType: { enumConstant: enumValue } } qemu_enums = {} # { enumType: { enumConstant: enumValue } } +event_ids = []
import os import sys @@ -219,6 +220,8 @@ def lxc_function(name, desc, ret, args, file, module, cond): def enum(type, name, value): if type not in enums: enums[type] = {} + if name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_'): + event_ids.append(name)
If you use brackets you can break the line without confusing python eg if (name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_')):
if value == 'VIR_TYPED_PARAM_INT': value = 1 elif value == 'VIR_TYPED_PARAM_UINT': @@ -910,10 +913,10 @@ def buildStubs(module, api_xml): wrapper_file = "build/%s.c" % module
include = open(header_file, "w") - include.write("/* Generated */\n\n") + include.write("/* Generated by generator.py */\n\n")
export = open(export_file, "w") - export.write("/* Generated */\n\n") + export.write("/* Generated by generator.py */\n\n")
wrapper = open(wrapper_file, "w") wrapper.write("/* Generated by generator.py */\n\n") @@ -943,6 +946,10 @@ def buildStubs(module, api_xml): # Write C pointer conversion functions. for classname in primary_classes: print_c_pointer(classname, wrapper, export, include) + # Write define wrappers around event id enums + include.write("\n") + for event_id in event_ids: + include.write("#define %s %s\n" % (event_id, event_id))
I'm not seeing the point of adding this define ?
include.close() export.close() diff --git a/libvirt-override.c b/libvirt-override.c index e51c44d..88cb527 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -4,7 +4,7 @@ * entry points where an automatically generated stub is * unpractical * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * Daniel Veillard <veillard@redhat.com> */ @@ -6346,7 +6346,7 @@ libvirt_virConnectDomainEventPMSuspendCallback(virConnectPtr conn ATTRIBUTE_UNUS }
-#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE static int libvirt_virConnectDomainEventBalloonChangeCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6398,9 +6398,9 @@ libvirt_virConnectDomainEventBalloonChangeCallback(virConnectPtr conn ATTRIBUTE_ LIBVIRT_RELEASE_THREAD_STATE; return ret; } -#endif /* LIBVIR_CHECK_VERSION(0, 10, 0) */ +#endif /* VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE */
-#if LIBVIR_CHECK_VERSION(1, 0, 0) +#ifdef VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK static int libvirt_virConnectDomainEventPMSuspendDiskCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6452,9 +6452,9 @@ libvirt_virConnectDomainEventPMSuspendDiskCallback(virConnectPtr conn ATTRIBUTE_ LIBVIRT_RELEASE_THREAD_STATE; return ret; } -#endif /* LIBVIR_CHECK_VERSION(1, 0, 0) */ +#endif /* VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK */
-#if LIBVIR_CHECK_VERSION(1, 1, 1) +#ifdef VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED static int libvirt_virConnectDomainEventDeviceRemovedCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6504,9 +6504,9 @@ libvirt_virConnectDomainEventDeviceRemovedCallback(virConnectPtr conn ATTRIBUTE_ LIBVIRT_RELEASE_THREAD_STATE; return ret; } -#endif /* LIBVIR_CHECK_VERSION(1, 1, 1) */ +#endif /* VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED */
-#if LIBVIR_CHECK_VERSION(1, 2, 9) +#ifdef VIR_DOMAIN_EVENT_ID_TUNABLE static int libvirt_virConnectDomainEventTunableCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6564,9 +6564,9 @@ libvirt_virConnectDomainEventTunableCallback(virConnectPtr conn ATTRIBUTE_UNUSED return ret;
} -#endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */ +#endif /* VIR_DOMAIN_EVENT_ID_TUNABLE */
-#if LIBVIR_CHECK_VERSION(1, 2, 11) +#ifdef VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE static int libvirt_virConnectDomainEventAgentLifecycleCallback(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainPtr dom, @@ -6618,7 +6618,7 @@ libvirt_virConnectDomainEventAgentLifecycleCallback(virConnectPtr conn ATTRIBUTE return ret;
} -#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ +#endif /* VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE */
static PyObject * @@ -6676,9 +6676,9 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject *self, cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventGenericCallback); break; case VIR_DOMAIN_EVENT_ID_BLOCK_JOB: -#if LIBVIR_CHECK_VERSION(1, 2, 6) +#ifdef VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 case VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2: -#endif /* LIBVIR_CHECK_VERSION(1, 2, 6) */ +#endif /* VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2 */ cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventBlockJobCallback); break; case VIR_DOMAIN_EVENT_ID_DISK_CHANGE: @@ -6693,31 +6693,31 @@ libvirt_virConnectDomainEventRegisterAny(ATTRIBUTE_UNUSED PyObject *self, case VIR_DOMAIN_EVENT_ID_PMSUSPEND: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventPMSuspendCallback); break; -#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE case VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventBalloonChangeCallback); break; -#endif /* LIBVIR_CHECK_VERSION(0, 10, 0) */ -#if LIBVIR_CHECK_VERSION(1, 0, 0) +#endif /* VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE */ +#ifdef VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK case VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventPMSuspendDiskCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 0, 0) */ -#if LIBVIR_CHECK_VERSION(1, 1, 1) +#endif /* VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK */ +#ifdef VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventDeviceRemovedCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 1, 1) */ -#if LIBVIR_CHECK_VERSION(1, 2, 9) +#endif /* VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED */ +#ifdef VIR_DOMAIN_EVENT_ID_TUNABLE case VIR_DOMAIN_EVENT_ID_TUNABLE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventTunableCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */ -#if LIBVIR_CHECK_VERSION(1, 2, 11) +#endif /* VIR_DOMAIN_EVENT_ID_TUNABLE */ +#ifdef VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE case VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE: cb = VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventAgentLifecycleCallback); break; -#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ +#endif /* VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE */ case VIR_DOMAIN_EVENT_ID_LAST: break; } -- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, 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 :|
On Thu, Jan 15, 2015 at 10:42:39 +0000, Daniel Berrange wrote:
On Wed, Jan 14, 2015 at 08:08:16PM -0700, Eric Blake wrote:
In some cases, it is very easy for downstream distros to backport enum values without requiring a .so bump. Keying the conditional code off of the upstream version where the enum value was added is not ideal, because downstream then has to patch that the feature is available in their build that still reports an earlier version number. For example, if RHEL 7 backports events from 1.2.11 into a build based on 1.2.8, building the python bindings would warn:
libvirt-override.c: In function ‘libvirt_virConnectDomainEventRegisterAny’: libvirt-override.c:6653:5: warning: enumeration value ‘VIR_DOMAIN_EVENT_ID_TUNABLE’ not handled in switch [-Wswitch] switch ((virDomainEventID) eventID) { ^ libvirt-override.c:6653:5: warning: enumeration value ‘VIR_DOMAIN_EVENT_ID_AGENT_LIFECYCLE’ not handled in switch [-Wswitch]
The solution is simple - use feature-based probes instead of version probes. Since we already scrape the XML API document of whatever libvirt build we are binding, and that XML already documents any downstream enum additions, we can use those as the features for gating conditional compilation.
* generator.py (enum): Track event id names. (buildStubs): Output define wrappers for events. * libvirt-override.c (libvirt_virConnectDomainEventBalloonChangeCallback) (libvirt_virConnectDomainEventPMSuspendDiskCallback) (libvirt_virConnectDomainEventDeviceRemovedCallback) (libvirt_virConnectDomainEventTunableCallback) (libvirt_virConnectDomainEventAgentLifecycleCallback) (libvirt_virConnectDomainEventRegisterAny): Use them.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
I mentioned this idea on the list a while back, and finally got time to code it up in a much nicer way than my initial attempt that would have polluted libvirt.h proper. https://www.redhat.com/archives/libvir-list/2014-October/msg00186.html
I'm relatively weak at python, and welcome to suggestions on how to avoid the long line in generator.py (maybe a regex match on VIR_.*_EVENT_ID_, but what's the canonical way to write that in python?)
generator.py | 11 +++++++++-- libvirt-override.c | 46 +++++++++++++++++++++++----------------------- 2 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/generator.py b/generator.py index cf044c9..4fd6d55 100755 --- a/generator.py +++ b/generator.py @@ -9,6 +9,7 @@ qemu_functions = {} enums = {} # { enumType: { enumConstant: enumValue } } lxc_enums = {} # { enumType: { enumConstant: enumValue } } qemu_enums = {} # { enumType: { enumConstant: enumValue } } +event_ids = []
import os import sys @@ -219,6 +220,8 @@ def lxc_function(name, desc, ret, args, file, module, cond): def enum(type, name, value): if type not in enums: enums[type] = {} + if name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_'): + event_ids.append(name)
If you use brackets you can break the line without confusing python eg
if (name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_')):
if value == 'VIR_TYPED_PARAM_INT': value = 1 elif value == 'VIR_TYPED_PARAM_UINT': @@ -910,10 +913,10 @@ def buildStubs(module, api_xml): wrapper_file = "build/%s.c" % module
include = open(header_file, "w") - include.write("/* Generated */\n\n") + include.write("/* Generated by generator.py */\n\n")
export = open(export_file, "w") - export.write("/* Generated */\n\n") + export.write("/* Generated by generator.py */\n\n")
wrapper = open(wrapper_file, "w") wrapper.write("/* Generated by generator.py */\n\n") @@ -943,6 +946,10 @@ def buildStubs(module, api_xml): # Write C pointer conversion functions. for classname in primary_classes: print_c_pointer(classname, wrapper, export, include) + # Write define wrappers around event id enums + include.write("\n") + for event_id in event_ids: + include.write("#define %s %s\n" % (event_id, event_id))
I'm not seeing the point of adding this define ? ...
-#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE
Without that define, you can't check for the events using ifdef, C preprocessor doesn't see enum items. Jirka
On 01/15/2015 03:51 AM, Jiri Denemark wrote:
On Thu, Jan 15, 2015 at 10:42:39 +0000, Daniel Berrange wrote:
On Wed, Jan 14, 2015 at 08:08:16PM -0700, Eric Blake wrote:
In some cases, it is very easy for downstream distros to backport enum values without requiring a .so bump. Keying the conditional code off of the upstream version where the enum value was added is not ideal, because downstream then has to patch that the feature is available in their build that still reports an earlier version number. For example, if RHEL 7 backports events from 1.2.11 into a build based on 1.2.8, building the python bindings would warn:
+ if name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_'): + event_ids.append(name)
If you use brackets you can break the line without confusing python eg
if (name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_')):
Thanks.
+ # Write define wrappers around event id enums + include.write("\n") + for event_id in event_ids: + include.write("#define %s %s\n" % (event_id, event_id))
I'm not seeing the point of adding this define ? ...
-#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE
Without that define, you can't check for the events using ifdef, C preprocessor doesn't see enum items.
Correct. Creating self-referential macros is the canonical way to convert enums into something that can be probed at preprocessor time for using conditional compilation. Since we have one macro for every enum that we read from the xml of the libvirt we are targetting, we can make the bindings support exactly the set of enum values that were backported into the particular libvirt, without having to worry about backporting crossing version number checks. So does this count as an ACK with the long line fixed? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Jan 15, 2015 at 08:52:44AM -0700, Eric Blake wrote:
On 01/15/2015 03:51 AM, Jiri Denemark wrote:
On Thu, Jan 15, 2015 at 10:42:39 +0000, Daniel Berrange wrote:
On Wed, Jan 14, 2015 at 08:08:16PM -0700, Eric Blake wrote:
In some cases, it is very easy for downstream distros to backport enum values without requiring a .so bump. Keying the conditional code off of the upstream version where the enum value was added is not ideal, because downstream then has to patch that the feature is available in their build that still reports an earlier version number. For example, if RHEL 7 backports events from 1.2.11 into a build based on 1.2.8, building the python bindings would warn:
+ if name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_'): + event_ids.append(name)
If you use brackets you can break the line without confusing python eg
if (name.startswith('VIR_DOMAIN_EVENT_ID_') or name.startswith('VIR_NETWORK_EVENT_ID_')):
Thanks.
+ # Write define wrappers around event id enums + include.write("\n") + for event_id in event_ids: + include.write("#define %s %s\n" % (event_id, event_id))
I'm not seeing the point of adding this define ? ...
-#if LIBVIR_CHECK_VERSION(0, 10, 0) +#ifdef VIR_DOMAIN_EVENT_ID_BALLOON_CHANGE
Without that define, you can't check for the events using ifdef, C preprocessor doesn't see enum items.
Correct. Creating self-referential macros is the canonical way to convert enums into something that can be probed at preprocessor time for using conditional compilation. Since we have one macro for every enum that we read from the xml of the libvirt we are targetting, we can make the bindings support exactly the set of enum values that were backported into the particular libvirt, without having to worry about backporting crossing version number checks.
Sigh. In a sane world we would just fix the CPP to honour enums directly ;-P
So does this count as an ACK with the long line fixed?
Sure, ACK Regards, 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 :|
On 01/15/2015 09:06 AM, Daniel P. Berrange wrote:
On Thu, Jan 15, 2015 at 08:52:44AM -0700, Eric Blake wrote:
On 01/15/2015 03:51 AM, Jiri Denemark wrote:
On Thu, Jan 15, 2015 at 10:42:39 +0000, Daniel Berrange wrote:
On Wed, Jan 14, 2015 at 08:08:16PM -0700, Eric Blake wrote:
In some cases, it is very easy for downstream distros to backport enum values without requiring a .so bump. Keying the conditional code off of the upstream version where the enum value was added is not ideal, because downstream then has to patch that the feature is available in their build that still reports an earlier version number. For example, if RHEL 7 backports events from 1.2.11 into a build based on 1.2.8, building the python bindings would warn:
Sigh. In a sane world we would just fix the CPP to honour enums directly ;-P
Yeah, the fact that the preprocessor and compiler are historically separate entities is baggage that we are stuck with.
So does this count as an ACK with the long line fixed?
Sure, ACK
Thanks; pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark