[libvirt] [RFC PATCH] include: make it easier to probe enum growth
https://bugzilla.redhat.com/show_bug.cgi?id=1147639 is an example of a downstream distro's dilemma - when backporting a feature that is implemented in an ABI-compatible manner (no .so bump was required) but where the feature involves new bits to be defined in a flags variable, how does one write code to reliably detect that those bits have been backported? The solution presented here is a common idiom used in a number of other header files (for example, glibc's /usr/include/langinfo.h does it for ABDAY_1 and friends); by adding a self-referential preprocessor macro, client code can easily do: | switch (state) { | #ifdef VIR_DOMAIN_PMSUSPENDED | case VIR_DOMAIN_PMSUSPENDED: | .... | #endif | } rather than trying to figure out which version number introduced VIR_DOMAIN_PMSUSPENDED (v.9.11), and using that with LIBVIR_CHECK_VERSION. Of course, since 1.2.10 would be the first release where this practice is reliable, we will still see clients that target earlier libvirt doing: | switch (state) { | #if LIBVIR_CHECK_VERSION(0, 9, 11) || defined(VIR_DOMAIN_PMSUSPENDED) | case VIR_DOMAIN_PMSUSPENDED: | .... | #endif | } but that is still more maintainable. * include/libvirt/libvirt.h.in (virDomainState): Expose #defines matching each enum value. Signed-off-by: Eric Blake <eblake@redhat.com> --- This patch is an RFC because I want confirmation that it is worth doing. Obviously, if it is desirable, there will be a LOT more addition of #define throughout the file, but as that is mostly busy-work, I want to get the idea approved first. include/libvirt/libvirt.h.in | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c910b31..0baea53 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -116,13 +116,28 @@ typedef virDomain *virDomainPtr; * A domain may be in different states at a given point in time */ typedef enum { +#define VIR_DOMAIN_NOSTATE VIR_DOMAIN_NOSTATE VIR_DOMAIN_NOSTATE = 0, /* no state */ + +#define VIR_DOMAIN_RUNNING VIR_DOMAIN_RUNNING VIR_DOMAIN_RUNNING = 1, /* the domain is running */ + +#define VIR_DOMAIN_BLOCKED VIR_DOMAIN_BLOCKED VIR_DOMAIN_BLOCKED = 2, /* the domain is blocked on resource */ + +#define VIR_DOMAIN_PAUSED VIR_DOMAIN_PAUSED VIR_DOMAIN_PAUSED = 3, /* the domain is paused by user */ + +#define VIR_DOMAIN_SHUTDOWN VIR_DOMAIN_SHUTDOWN VIR_DOMAIN_SHUTDOWN= 4, /* the domain is being shut down */ + +#define VIR_DOMAIN_SHUTOFF VIR_DOMAIN_SHUTOFF VIR_DOMAIN_SHUTOFF = 5, /* the domain is shut off */ + +#define VIR_DOMAIN_CRASHED VIR_DOMAIN_CRASHED VIR_DOMAIN_CRASHED = 6, /* the domain is crashed */ + +#define VIR_DOMAIN_PMSUSPENDED VIR_DOMAIN_PMSUSPENDED VIR_DOMAIN_PMSUSPENDED = 7, /* the domain is suspended by guest power management */ -- 1.9.3
On Mon, Oct 06, 2014 at 02:06:56PM -0600, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1147639 is an example of a downstream distro's dilemma - when backporting a feature that is implemented in an ABI-compatible manner (no .so bump was required) but where the feature involves new bits to be defined in a flags variable, how does one write code to reliably detect that those bits have been backported?
My answer would be that distros shouldn't be cherry-picking bits of the public header file at all so they don't create these non-standard APIs.
The solution presented here is a common idiom used in a number of other header files (for example, glibc's /usr/include/langinfo.h does it for ABDAY_1 and friends); by adding a self-referential preprocessor macro, client code can easily do:
| switch (state) { | #ifdef VIR_DOMAIN_PMSUSPENDED | case VIR_DOMAIN_PMSUSPENDED: | .... | #endif | }
rather than trying to figure out which version number introduced VIR_DOMAIN_PMSUSPENDED (v.9.11), and using that with LIBVIR_CHECK_VERSION. Of course, since 1.2.10 would be the first release where this practice is reliable, we will still see clients that target earlier libvirt doing:
| switch (state) { | #if LIBVIR_CHECK_VERSION(0, 9, 11) || defined(VIR_DOMAIN_PMSUSPENDED) | case VIR_DOMAIN_PMSUSPENDED: | .... | #endif | }
but that is still more maintainable.
* include/libvirt/libvirt.h.in (virDomainState): Expose #defines matching each enum value.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
This patch is an RFC because I want confirmation that it is worth doing. Obviously, if it is desirable, there will be a LOT more addition of #define throughout the file, but as that is mostly busy-work, I want to get the idea approved first.
include/libvirt/libvirt.h.in | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c910b31..0baea53 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -116,13 +116,28 @@ typedef virDomain *virDomainPtr; * A domain may be in different states at a given point in time */ typedef enum { +#define VIR_DOMAIN_NOSTATE VIR_DOMAIN_NOSTATE VIR_DOMAIN_NOSTATE = 0, /* no state */ + +#define VIR_DOMAIN_RUNNING VIR_DOMAIN_RUNNING VIR_DOMAIN_RUNNING = 1, /* the domain is running */ + +#define VIR_DOMAIN_BLOCKED VIR_DOMAIN_BLOCKED VIR_DOMAIN_BLOCKED = 2, /* the domain is blocked on resource */ + +#define VIR_DOMAIN_PAUSED VIR_DOMAIN_PAUSED VIR_DOMAIN_PAUSED = 3, /* the domain is paused by user */ + +#define VIR_DOMAIN_SHUTDOWN VIR_DOMAIN_SHUTDOWN VIR_DOMAIN_SHUTDOWN= 4, /* the domain is being shut down */ + +#define VIR_DOMAIN_SHUTOFF VIR_DOMAIN_SHUTOFF VIR_DOMAIN_SHUTOFF = 5, /* the domain is shut off */ + +#define VIR_DOMAIN_CRASHED VIR_DOMAIN_CRASHED VIR_DOMAIN_CRASHED = 6, /* the domain is crashed */ + +#define VIR_DOMAIN_PMSUSPENDED VIR_DOMAIN_PMSUSPENDED VIR_DOMAIN_PMSUSPENDED = 7, /* the domain is suspended by guest power management */
This is pretty damn ugly IMHO. I'd only support that if it was entirely automatically generated as part of the libvirt.h.in -> libvirt.h conversion. 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 10/07/2014 03:16 AM, Daniel P. Berrange wrote:
On Mon, Oct 06, 2014 at 02:06:56PM -0600, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1147639 is an example of a downstream distro's dilemma - when backporting a feature that is implemented in an ABI-compatible manner (no .so bump was required) but where the feature involves new bits to be defined in a flags variable, how does one write code to reliably detect that those bits have been backported?
My answer would be that distros shouldn't be cherry-picking bits of the public header file at all so they don't create these non-standard APIs.
That ship has already sailed. Looking at just RHEL 7.0, I see the following bits have been backported: VIR_MIGRAGE_PARAM_LISTEN_ADDRESS (already a #define) VIR_STORAGE_VOL_NETDIR VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START or looking at RHEL 6.5: VIR_DOMAIN_RUNNING_CRASHED VIR_DOMAIN_PAUSED_SNAPSHOT VIR_DOMAIN_PAUSED_CRASHED VIR_DOMAIN_CRASHED_PANICKED VIR_DOMAIN_PMSUSPENDED_DISK_UNKNOWN VIR_MIGRATE_ABORT_ON_ERROR VIR_DOMAIN_XML_MIGRATABLE VIR_DOMAIN_EVENT_CRASHED VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR VIR_DOMAIN_EVENT_PMSUSPENDED_DISK VIR_DOMAIN_EVENT_CRASHED_PANICKED VIR_DOMAIN_SNAPSHOT_CREATE_LIVE VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL VIR_DOMAIN_BLOCK_JOB_READY VIR_DOMAIN_EVENT_ID_PMSUSPEND_DISK VIR_DOMAIN_MEMORY_SHARED_MERGE_ACROSS_NODES (already a #define) In other words, it's a VERY common action for downstreams to conditionally backport new feature bits of existing APIs. Downstream language bindings needs to reflect these additional bits. In the past, when libvirt-python was part of libvirt.git, the python bindings got this for free. But the bugzilla I mentioned above is a case where RHEL 7.1 is considering the backport of a new VIR_DOMAIN_EVENT_ID_* bit where gating on version alone is insufficient for writing correct libvirt python bindings; since the python bindings live in a different repository and need SOME witness of whether the event has been backported, since it is easy to backport new events without breaking ABI. Writing the upstream libvirt-python bindings to use witness variables instead of version numbers as the gate for any use of a particular feature (especially VIR_DOMAIN_EVENT_ID_*) makes it more robust to build out-of-the-box with a downstream libvirt that has backported feature bits. On the other hand, the python bindings have already solved most of the backport issue by scraping the XML document of all exported bits as generated by the downstream libvirt, so new flags bits automatically get exposed by the code generators. I guess it may require auditing the libvirt-python bindings to see all use of LIBVIR_CHECK_VERSION to see which uses are NOT tied to an ABI change, and maybe special-case that hand-written C code to instead play off of something that the generator can produce automatically based off of what it scrapes from the XML. If THAT works, then I could patch just libvirt-python without worrying about uglifying libvirt.h.
The solution presented here is a common idiom used in a number of other header files (for example, glibc's /usr/include/langinfo.h does it for ABDAY_1 and friends); by adding a self-referential preprocessor macro,
+ +#define VIR_DOMAIN_PMSUSPENDED VIR_DOMAIN_PMSUSPENDED VIR_DOMAIN_PMSUSPENDED = 7, /* the domain is suspended by guest power management */
This is pretty damn ugly IMHO. I'd only support that if it was entirely automatically generated as part of the libvirt.h.in -> libvirt.h conversion.
That's a fair point. If my experiments with trying to solve the issue from the libvirt-python side don't pan out, then I'll look at what it would take to automate things, so that it is not a maintainer burden. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake