[libvirt] [PATCH 0/2] consistent use of _LAST enum markers

This patch series will make it harder to add an enum value while forgetting to translate that enum to or from an appropriate string value. It also alters the public API so that users don't get _LAST enum values unless they ask for them, since such values are markers that might change over time rather than an official unchanging API value. Eric Blake (2): API: make declaration of _LAST enum values conditional maint: enforce use of _LAST marker cfg.mk | 12 +++- daemon/libvirtd.h | 8 +- include/libvirt/libvirt.h.in | 186 ++++++++++++++++++++++++++++++++++++----- python/generator.py | 3 +- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 3 +- src/internal.h | 4 + src/qemu/qemu_monitor_json.c | 9 +- src/remote/remote_protocol.x | 3 +- src/util/virtypedparam.c | 2 +- tests/cputest.c | 3 +- tools/virsh.c | 9 ++ 12 files changed, 207 insertions(+), 37 deletions(-) -- 1.7.7.5

Although this is a public API break, it only affects users that were compiling against *_LAST values, and can be trivially worked around without impacting compilation against older headers, by the user defining LIBVIRT_ENUM_SENTINELS before including libvirt.h. It is not an ABI break, since enum values do not appear as .so entry points. See this list discussion: https://www.redhat.com/archives/libvir-list/2012-January/msg00804.html * include/libvirt/libvirt.h.in: Hide all sentinels behind LIBVIRT_ENUM_SENTINELS, and add missing sentinels. * src/internal.h (VIR_DEPRECATED): Allow inclusion after libvirt.h. (LIBVIRT_ENUM_SENTINELS): Expose sentinels internally. * daemon/libvirtd.h: Use the sentinels. * src/remote/remote_protocol.x (includes): Don't expose sentinels. * python/generator.py (enum): Likewise. * tests/cputest.c (cpuTestCompResStr): Silence compiler warning. * tools/virsh.c (vshDomainStateReasonToString) (vshDomainControlStateToString): Likewise. --- daemon/libvirtd.h | 8 +- include/libvirt/libvirt.h.in | 186 ++++++++++++++++++++++++++++++++++++----- python/generator.py | 3 +- src/internal.h | 4 + src/remote/remote_protocol.x | 3 +- tests/cputest.c | 3 +- tools/virsh.c | 9 ++ 7 files changed, 187 insertions(+), 29 deletions(-) diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h index c8d3ca2..8a4136c 100644 --- a/daemon/libvirtd.h +++ b/daemon/libvirtd.h @@ -1,7 +1,7 @@ /* * libvirtd.h: daemon data structure definitions * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -22,8 +22,10 @@ */ -#ifndef QEMUD_INTERNAL_H__ -# define QEMUD_INTERNAL_H__ +#ifndef LIBVIRTD_H__ +# define LIBVIRTD_H__ + +# define LIBVIRT_ENUM_SENTINELS # include <config.h> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e436f3c..e8300f5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -43,6 +43,12 @@ extern "C" { # define VIR_EXPORT_VAR extern #endif +/* General note - throughout this file, any linear enumeration which + * might be expanded in the future has an optional *_LAST value that + * gives the size of the enum at the time of compilation, if the user + * defines LIBVIRT_ENUM_SENTINELS. Enumerations for bit values do not + * have a *_LAST value, but additional bits may be defined. */ + /** * virConnect: * @@ -88,16 +94,22 @@ typedef enum { VIR_DOMAIN_SHUTOFF = 5, /* the domain is shut off */ VIR_DOMAIN_CRASHED = 6, /* the domain is crashed */ +#ifdef LIBVIRT_ENUM_SENTINELS /* * NB: this enum value will increase over time as new events are * added to the libvirt API. It reflects the last state supported * by this version of the libvirt API. */ VIR_DOMAIN_LAST +#endif } virDomainState; typedef enum { VIR_DOMAIN_NOSTATE_UNKNOWN = 0, + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_NOSTATE_LAST +#endif } virDomainNostateReason; typedef enum { @@ -109,10 +121,18 @@ typedef enum { VIR_DOMAIN_RUNNING_UNPAUSED = 5, /* returned from paused state */ VIR_DOMAIN_RUNNING_MIGRATION_CANCELED = 6, /* returned from migration */ VIR_DOMAIN_RUNNING_SAVE_CANCELED = 7, /* returned from failed save process */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_RUNNING_LAST +#endif } virDomainRunningReason; typedef enum { VIR_DOMAIN_BLOCKED_UNKNOWN = 0, /* the reason is unknown */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_BLOCKED_LAST +#endif } virDomainBlockedReason; typedef enum { @@ -125,11 +145,19 @@ typedef enum { VIR_DOMAIN_PAUSED_WATCHDOG = 6, /* paused due to a watchdog event */ VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* paused after restoring from snapshot */ VIR_DOMAIN_PAUSED_SHUTTING_DOWN = 8, /* paused during shutdown process */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_PAUSED_LAST +#endif } virDomainPausedReason; typedef enum { VIR_DOMAIN_SHUTDOWN_UNKNOWN = 0, /* the reason is unknown */ VIR_DOMAIN_SHUTDOWN_USER = 1, /* shutting down on user request */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_SHUTDOWN_LAST +#endif } virDomainShutdownReason; typedef enum { @@ -142,10 +170,17 @@ typedef enum { VIR_DOMAIN_SHUTOFF_FAILED = 6, /* domain failed to start */ VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT = 7, /* restored from a snapshot which was * taken while domain was shutoff */ +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_SHUTOFF_LAST +#endif } virDomainShutoffReason; typedef enum { VIR_DOMAIN_CRASHED_UNKNOWN = 0, /* crashed for unknown reason */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_CRASHED_LAST +#endif } virDomainCrashedReason; @@ -161,6 +196,10 @@ typedef enum { limited set of commands may be allowed */ VIR_DOMAIN_CONTROL_OCCUPIED = 2, /* occupied by a running command */ VIR_DOMAIN_CONTROL_ERROR = 3, /* unusable, domain cannot be fully operated */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_CONTROL_LAST +#endif } virDomainControlState; /** @@ -261,8 +300,10 @@ typedef enum { VIR_NODE_SUSPEND_TARGET_DISK = 1, VIR_NODE_SUSPEND_TARGET_HYBRID = 2, +#ifdef LIBVIRT_ENUM_SENTINELS /* This constant is subject to change */ VIR_NODE_SUSPEND_TARGET_LAST, +#endif } virNodeSuspendTarget; /** @@ -512,6 +553,10 @@ typedef enum { VIR_TYPED_PARAM_DOUBLE = 5, /* double case */ VIR_TYPED_PARAM_BOOLEAN = 6, /* boolean(character) case */ VIR_TYPED_PARAM_STRING = 7, /* string case */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_TYPED_PARAM_LAST +#endif } virTypedParameterType; /** @@ -844,6 +889,10 @@ typedef enum { * To add new statistics, add them to the enum and increase this value. */ VIR_DOMAIN_MEMORY_STAT_NR = 7, + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR +#endif } virDomainMemoryStatTags; typedef struct _virDomainMemoryStat virDomainMemoryStatStruct; @@ -973,7 +1022,10 @@ typedef enum { VIR_CRED_REALM = 8, /* Authentication realm */ VIR_CRED_EXTERNAL = 9, /* Externally managed credential */ +#ifdef LIBVIRT_ENUM_SENTINELS /* More may be added - expect the unexpected */ + VIR_CRED_LAST +#endif } virConnectCredentialType; struct _virConnectCredential { @@ -1356,8 +1408,10 @@ typedef enum { VIR_DOMAIN_NUMATUNE_MEM_PREFERRED = 1, VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE = 2, +#ifdef LIBVIRT_ENUM_SENTINELS /* This constant is subject to change */ VIR_DOMAIN_NUMATUNE_MEM_LAST +#endif } virDomainNumatuneMemMode; /** @@ -1558,8 +1612,8 @@ int virDomainMemoryStats (virDomainPtr dom, /* Memory peeking flags. */ typedef enum { - VIR_MEMORY_VIRTUAL = 1, /* addresses are virtual addresses */ - VIR_MEMORY_PHYSICAL = 2, /* addresses are physical addresses */ + VIR_MEMORY_VIRTUAL = 1 << 0, /* addresses are virtual addresses */ + VIR_MEMORY_PHYSICAL = 1 << 1, /* addresses are physical addresses */ } virDomainMemoryFlags; int virDomainMemoryPeek (virDomainPtr dom, @@ -1609,6 +1663,10 @@ typedef enum { VIR_VCPU_OFFLINE = 0, /* the virtual CPU is offline */ VIR_VCPU_RUNNING = 1, /* the virtual CPU is running */ VIR_VCPU_BLOCKED = 2, /* the virtual CPU is blocked on resource */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_VCPU_LAST +#endif } virVcpuState; typedef struct _virVcpuInfo virVcpuInfo; @@ -1774,6 +1832,10 @@ int virDomainUpdateDeviceFlags(virDomainPtr domain, typedef enum { VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1, + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_BLOCK_JOB_TYPE_LAST +#endif } virDomainBlockJobType; /* An iterator for monitoring block job operations */ @@ -2018,7 +2080,7 @@ const char* virInterfaceGetName (virInterfacePtr iface); const char* virInterfaceGetMACString (virInterfacePtr iface); typedef enum { - VIR_INTERFACE_XML_INACTIVE = 1 /* dump inactive interface information */ + VIR_INTERFACE_XML_INACTIVE = 1 << 0 /* dump inactive interface information */ } virInterfaceXMLFlags; char * virInterfaceGetXMLDesc (virInterfacePtr iface, @@ -2062,25 +2124,29 @@ typedef virStoragePool *virStoragePoolPtr; typedef enum { - VIR_STORAGE_POOL_INACTIVE = 0, /* Not running */ - VIR_STORAGE_POOL_BUILDING = 1, /* Initializing pool, not available */ - VIR_STORAGE_POOL_RUNNING = 2, /* Running normally */ - VIR_STORAGE_POOL_DEGRADED = 3, /* Running degraded */ - VIR_STORAGE_POOL_INACCESSIBLE = 4, /* Running, but not accessible */ + VIR_STORAGE_POOL_INACTIVE = 0, /* Not running */ + VIR_STORAGE_POOL_BUILDING = 1, /* Initializing pool, not available */ + VIR_STORAGE_POOL_RUNNING = 2, /* Running normally */ + VIR_STORAGE_POOL_DEGRADED = 3, /* Running degraded */ + VIR_STORAGE_POOL_INACCESSIBLE = 4, /* Running, but not accessible */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_STORAGE_POOL_STATE_LAST +#endif } virStoragePoolState; typedef enum { - VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */ - VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */ - VIR_STORAGE_POOL_BUILD_RESIZE = (1 << 1), /* Extend existing pool */ - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE = (1 << 2), /* Do not overwrite existing pool */ - VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3), /* Overwrite data */ + VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */ + VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */ + VIR_STORAGE_POOL_BUILD_RESIZE = (1 << 1), /* Extend existing pool */ + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE = (1 << 2), /* Do not overwrite existing pool */ + VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3), /* Overwrite data */ } virStoragePoolBuildFlags; typedef enum { VIR_STORAGE_POOL_DELETE_NORMAL = 0, /* Delete metadata only (fast) */ - VIR_STORAGE_POOL_DELETE_ZEROED = 1, /* Clear all data to zeros (slow) */ + VIR_STORAGE_POOL_DELETE_ZEROED = 1 << 0, /* Clear all data to zeros (slow) */ } virStoragePoolDeleteFlags; typedef struct _virStoragePoolInfo virStoragePoolInfo; @@ -2115,11 +2181,15 @@ typedef enum { VIR_STORAGE_VOL_FILE = 0, /* Regular file based volumes */ VIR_STORAGE_VOL_BLOCK = 1, /* Block based volumes */ VIR_STORAGE_VOL_DIR = 2, /* Directory-passthrough based volume */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_STORAGE_VOL_LAST +#endif } virStorageVolType; typedef enum { VIR_STORAGE_VOL_DELETE_NORMAL = 0, /* Delete metadata only (fast) */ - VIR_STORAGE_VOL_DELETE_ZEROED = 1, /* Clear all data to zeros (slow) */ + VIR_STORAGE_VOL_DELETE_ZEROED = 1 << 0, /* Clear all data to zeros (slow) */ } virStorageVolDeleteFlags; typedef struct _virStorageVolInfo virStorageVolInfo; @@ -2286,12 +2356,14 @@ typedef enum { VIR_KEYCODE_SET_WIN32 = 8, VIR_KEYCODE_SET_RFB = 9, +#ifdef LIBVIRT_ENUM_SENTINELS /* * NB: this enum value will increase over time as new events are * added to the libvirt API. It reflects the last keycode set supported * by this version of the libvirt API. */ VIR_KEYCODE_SET_LAST, +#endif } virKeycodeSet; /** @@ -2389,13 +2461,17 @@ int virNodeDeviceDestroy (virNodeDevicePtr dev); * a virDomainEventType is emitted during domain lifecycle events */ typedef enum { - VIR_DOMAIN_EVENT_DEFINED = 0, - VIR_DOMAIN_EVENT_UNDEFINED = 1, - VIR_DOMAIN_EVENT_STARTED = 2, - VIR_DOMAIN_EVENT_SUSPENDED = 3, - VIR_DOMAIN_EVENT_RESUMED = 4, - VIR_DOMAIN_EVENT_STOPPED = 5, - VIR_DOMAIN_EVENT_SHUTDOWN = 6, + VIR_DOMAIN_EVENT_DEFINED = 0, + VIR_DOMAIN_EVENT_UNDEFINED = 1, + VIR_DOMAIN_EVENT_STARTED = 2, + VIR_DOMAIN_EVENT_SUSPENDED = 3, + VIR_DOMAIN_EVENT_RESUMED = 4, + VIR_DOMAIN_EVENT_STOPPED = 5, + VIR_DOMAIN_EVENT_SHUTDOWN = 6, + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_LAST +#endif } virDomainEventType; /** @@ -2406,6 +2482,10 @@ typedef enum { typedef enum { VIR_DOMAIN_EVENT_DEFINED_ADDED = 0, /* Newly created config file */ VIR_DOMAIN_EVENT_DEFINED_UPDATED = 1, /* Changed config file */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_DEFINED_LAST +#endif } virDomainEventDefinedDetailType; /** @@ -2415,6 +2495,10 @@ typedef enum { */ typedef enum { VIR_DOMAIN_EVENT_UNDEFINED_REMOVED = 0, /* Deleted the config file */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_UNDEFINED_LAST +#endif } virDomainEventUndefinedDetailType; /** @@ -2427,6 +2511,10 @@ typedef enum { VIR_DOMAIN_EVENT_STARTED_MIGRATED = 1, /* Incoming migration from another host */ VIR_DOMAIN_EVENT_STARTED_RESTORED = 2, /* Restored from a state file */ VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from snapshot */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_STARTED_LAST +#endif } virDomainEventStartedDetailType; /** @@ -2441,6 +2529,10 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG = 3, /* Suspended due to a watchdog firing */ VIR_DOMAIN_EVENT_SUSPENDED_RESTORED = 4, /* Restored from paused state file */ VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT = 5, /* Restored from paused snapshot */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_SUSPENDED_LAST +#endif } virDomainEventSuspendedDetailType; /** @@ -2452,6 +2544,10 @@ typedef enum { VIR_DOMAIN_EVENT_RESUMED_UNPAUSED = 0, /* Normal resume due to admin unpause */ VIR_DOMAIN_EVENT_RESUMED_MIGRATED = 1, /* Resumed for completion of migration */ VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT = 2, /* Resumed from snapshot */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_RESUMED_LAST +#endif } virDomainEventResumedDetailType; /** @@ -2467,6 +2563,10 @@ typedef enum { VIR_DOMAIN_EVENT_STOPPED_SAVED = 4, /* Saved to a state file */ VIR_DOMAIN_EVENT_STOPPED_FAILED = 5, /* Host emulator/mgmt failed */ VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT = 6, /* offline snapshot loaded */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_STOPPED_LAST +#endif } virDomainEventStoppedDetailType; @@ -2477,6 +2577,10 @@ typedef enum { */ typedef enum { VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, /* Guest finished shutdown sequence */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_SHUTDOWN_LAST +#endif } virDomainEventShutdownDetailType; /** @@ -2690,12 +2794,14 @@ typedef enum { VIR_SECRET_USAGE_TYPE_VOLUME = 1, VIR_SECRET_USAGE_TYPE_CEPH = 2, +#ifdef LIBVIRT_ENUM_SENTINELS /* * NB: this enum value will increase over time as new events are * added to the libvirt API. It reflects the last secret owner ID * supported by this version of the libvirt API. */ VIR_SECRET_USAGE_TYPE_LAST +#endif } virSecretUsageType; virConnectPtr virSecretGetConnect (virSecretPtr secret); @@ -2877,7 +2983,11 @@ typedef enum { VIR_CPU_COMPARE_ERROR = -1, VIR_CPU_COMPARE_INCOMPATIBLE = 0, VIR_CPU_COMPARE_IDENTICAL = 1, - VIR_CPU_COMPARE_SUPERSET = 2 + VIR_CPU_COMPARE_SUPERSET = 2, + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_CPU_COMPARE_LAST +#endif } virCPUCompareResult; /** @@ -2921,6 +3031,10 @@ typedef enum { VIR_DOMAIN_JOB_COMPLETED = 3, /* Job has finished, but isn't cleaned up */ VIR_DOMAIN_JOB_FAILED = 4, /* Job hit error, but isn't cleaned up */ VIR_DOMAIN_JOB_CANCELLED = 5, /* Job was aborted, but isn't cleaned up */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_JOB_LAST +#endif } virDomainJobType; typedef struct _virDomainJobInfo virDomainJobInfo; @@ -3121,6 +3235,10 @@ typedef enum { VIR_DOMAIN_EVENT_WATCHDOG_POWEROFF, /* Guest is forcably powered off */ VIR_DOMAIN_EVENT_WATCHDOG_SHUTDOWN, /* Guest is requested to gracefully shutdown */ VIR_DOMAIN_EVENT_WATCHDOG_DEBUG, /* No action, a debug message logged */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_WATCHDOG_LAST +#endif } virDomainEventWatchdogAction; /** @@ -3148,6 +3266,10 @@ typedef enum { VIR_DOMAIN_EVENT_IO_ERROR_NONE = 0, /* No action, IO error ignored */ VIR_DOMAIN_EVENT_IO_ERROR_PAUSE, /* Guest CPUs are pausde */ VIR_DOMAIN_EVENT_IO_ERROR_REPORT, /* IO error reported to guest OS */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_IO_ERROR_LAST +#endif } virDomainEventIOErrorAction; @@ -3201,6 +3323,10 @@ typedef enum { VIR_DOMAIN_EVENT_GRAPHICS_CONNECT = 0, /* Initial socket connection established */ VIR_DOMAIN_EVENT_GRAPHICS_INITIALIZE, /* Authentication & setup completed */ VIR_DOMAIN_EVENT_GRAPHICS_DISCONNECT, /* Final socket disconnection */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_GRAPHICS_LAST +#endif } virDomainEventGraphicsPhase; /** @@ -3212,6 +3338,10 @@ typedef enum { VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_IPV4, /* IPv4 address */ VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_IPV6, /* IPv6 address */ VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_UNIX, /* UNIX socket path */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_LAST +#endif } virDomainEventGraphicsAddressType; @@ -3293,6 +3423,10 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn, typedef enum { VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0, VIR_DOMAIN_BLOCK_JOB_FAILED = 1, + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_BLOCK_JOB_LAST +#endif } virConnectDomainEventBlockJobStatus; /** @@ -3320,6 +3454,10 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, */ typedef enum { VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ + +#ifdef LIBVIRT_ENUM_SENTINELS + VIR_DOMAIN_EVENT_DISK_CHANGE_LAST +#endif } virConnectDomainEventDiskChangeReason; /** @@ -3369,12 +3507,14 @@ typedef enum { VIR_DOMAIN_EVENT_ID_BLOCK_JOB = 8, /* virConnectDomainEventBlockJobCallback */ VIR_DOMAIN_EVENT_ID_DISK_CHANGE = 9, /* virConnectDomainEventDiskChangeCallback */ +#ifdef LIBVIRT_ENUM_SENTINELS /* * NB: this enum value will increase over time as new events are * added to the libvirt API. It reflects the last event ID supported * by this version of the libvirt API. */ VIR_DOMAIN_EVENT_ID_LAST +#endif } virDomainEventID; diff --git a/python/generator.py b/python/generator.py index 6fee3a4..de635dc 100755 --- a/python/generator.py +++ b/python/generator.py @@ -205,7 +205,8 @@ def enum(type, name, value): value = 1 elif value == 'VIR_DOMAIN_AFFECT_CONFIG': value = 2 - enums[type][name] = value + if name[-5:] != '_LAST': + enums[type][name] = value def qemu_enum(type, name, value): if not qemu_enums.has_key(type): diff --git a/src/internal.h b/src/internal.h index 4184f14..08148a9 100644 --- a/src/internal.h +++ b/src/internal.h @@ -22,8 +22,12 @@ * variables, so effectively undefine the deprecated attribute * which would otherwise be defined in libvirt.h. */ +# undef VIR_DEPRECATED # define VIR_DEPRECATED /*empty*/ +/* The library itself needs to know enum sizes. */ +# define LIBVIRT_ENUM_SENTINELS + /* All uses of _() within the library should pick up translations from * libvirt's message files, rather than from the package that is * linking in the library. Setting this macro before including diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ca739ff..514b0cc 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3,7 +3,7 @@ * remote_internal driver and libvirtd. This protocol is * internal and may change at any time. * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -36,6 +36,7 @@ * 'REMOTE_'. This makes names quite long. */ +%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h> diff --git a/tests/cputest.c b/tests/cputest.c index 938e087..2bfac88 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -1,7 +1,7 @@ /* * cputest.c: Test the libvirtd internal CPU APIs * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -205,6 +205,7 @@ cpuTestCompResStr(virCPUCompareResult result) case VIR_CPU_COMPARE_INCOMPATIBLE: return "INCOMPATIBLE"; case VIR_CPU_COMPARE_IDENTICAL: return "IDENTICAL"; case VIR_CPU_COMPARE_SUPERSET: return "SUPERSET"; + default: ; } return "unknown"; diff --git a/tools/virsh.c b/tools/virsh.c index 032a4e0..fd378a6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -17574,6 +17574,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_NOSTATE: switch ((virDomainNostateReason) reason) { case VIR_DOMAIN_NOSTATE_UNKNOWN: + default: ; } break; @@ -17595,6 +17596,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_RUNNING_SAVE_CANCELED: return N_("save canceled"); case VIR_DOMAIN_RUNNING_UNKNOWN: + default: ; } break; @@ -17602,6 +17604,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_BLOCKED: switch ((virDomainBlockedReason) reason) { case VIR_DOMAIN_BLOCKED_UNKNOWN: + default: ; } break; @@ -17625,6 +17628,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_PAUSED_SHUTTING_DOWN: return N_("shutting down"); case VIR_DOMAIN_PAUSED_UNKNOWN: + default: ; } break; @@ -17634,6 +17638,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_SHUTDOWN_USER: return N_("user"); case VIR_DOMAIN_SHUTDOWN_UNKNOWN: + default: ; } break; @@ -17655,6 +17660,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT: return N_("from snapshot"); case VIR_DOMAIN_SHUTOFF_UNKNOWN: + default: ; } break; @@ -17662,6 +17668,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_CRASHED: switch ((virDomainCrashedReason) reason) { case VIR_DOMAIN_CRASHED_UNKNOWN: + default: ; } break; @@ -17753,6 +17760,8 @@ vshDomainControlStateToString(int state) return N_("occupied"); case VIR_DOMAIN_CONTROL_ERROR: return N_("error"); + default: + ; } return N_("unknown"); -- 1.7.7.5

On Fri, Jan 20, 2012 at 14:06:26 -0700, Eric Blake wrote:
Although this is a public API break, it only affects users that were compiling against *_LAST values, and can be trivially worked around without impacting compilation against older headers, by the user defining LIBVIRT_ENUM_SENTINELS before including libvirt.h. It is not an ABI break, since enum values do not appear as .so entry points.
See this list discussion: https://www.redhat.com/archives/libvir-list/2012-January/msg00804.html
* include/libvirt/libvirt.h.in: Hide all sentinels behind LIBVIRT_ENUM_SENTINELS, and add missing sentinels. * src/internal.h (VIR_DEPRECATED): Allow inclusion after libvirt.h. (LIBVIRT_ENUM_SENTINELS): Expose sentinels internally. * daemon/libvirtd.h: Use the sentinels. * src/remote/remote_protocol.x (includes): Don't expose sentinels. * python/generator.py (enum): Likewise. * tests/cputest.c (cpuTestCompResStr): Silence compiler warning. * tools/virsh.c (vshDomainStateReasonToString) (vshDomainControlStateToString): Likewise. --- daemon/libvirtd.h | 8 +- include/libvirt/libvirt.h.in | 186 ++++++++++++++++++++++++++++++++++++----- python/generator.py | 3 +- src/internal.h | 4 + src/remote/remote_protocol.x | 3 +- tests/cputest.c | 3 +- tools/virsh.c | 9 ++ 7 files changed, 187 insertions(+), 29 deletions(-)
Overall, I like the patch but I don't agree with the way of silencing compilation warnings in the second part. The main benefit of avoiding default in switches used with enums is that the compiler is nice and warns us when new item is added to an enum. I'd prefer not to ruin this. ...
diff --git a/tests/cputest.c b/tests/cputest.c index 938e087..2bfac88 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -1,7 +1,7 @@ /* * cputest.c: Test the libvirtd internal CPU APIs * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -205,6 +205,7 @@ cpuTestCompResStr(virCPUCompareResult result) case VIR_CPU_COMPARE_INCOMPATIBLE: return "INCOMPATIBLE"; case VIR_CPU_COMPARE_IDENTICAL: return "IDENTICAL"; case VIR_CPU_COMPARE_SUPERSET: return "SUPERSET"; + default: ; case VIR_CPU_COMPARE_LAST: ; }
return "unknown"; diff --git a/tools/virsh.c b/tools/virsh.c index 032a4e0..fd378a6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -17574,6 +17574,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_NOSTATE: switch ((virDomainNostateReason) reason) { case VIR_DOMAIN_NOSTATE_UNKNOWN: + default: case VIR_DOMAIN_NOSTATE_LAST: ; } break; @@ -17595,6 +17596,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_RUNNING_SAVE_CANCELED: return N_("save canceled"); case VIR_DOMAIN_RUNNING_UNKNOWN: + default: case VIR_DOMAIN_RUNNING_LAST: ; } break; @@ -17602,6 +17604,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_BLOCKED: switch ((virDomainBlockedReason) reason) { case VIR_DOMAIN_BLOCKED_UNKNOWN: + default: case VIR_DOMAIN_BLOCKED_LAST: ; } break; @@ -17625,6 +17628,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_PAUSED_SHUTTING_DOWN: return N_("shutting down"); case VIR_DOMAIN_PAUSED_UNKNOWN: + default: case VIR_DOMAIN_PAUSED_LAST: ; } break; @@ -17634,6 +17638,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_SHUTDOWN_USER: return N_("user"); case VIR_DOMAIN_SHUTDOWN_UNKNOWN: + default: case VIR_DOMAIN_SHUTDOWN_LAST: ; } break; @@ -17655,6 +17660,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT: return N_("from snapshot"); case VIR_DOMAIN_SHUTOFF_UNKNOWN: + default: case VIR_DOMAIN_SHUTOFF_LAST: ; } break; @@ -17662,6 +17668,7 @@ vshDomainStateReasonToString(int state, int reason) case VIR_DOMAIN_CRASHED: switch ((virDomainCrashedReason) reason) { case VIR_DOMAIN_CRASHED_UNKNOWN: + default: case VIR_DOMAIN_CRASHED_LAST: ; } break; @@ -17753,6 +17760,8 @@ vshDomainControlStateToString(int state) return N_("occupied"); case VIR_DOMAIN_CONTROL_ERROR: return N_("error"); + default: case VIR_DOMAIN_CONTROL_LAST: + ; }
return N_("unknown");
ACK with those defaults removed. Jirka

On 01/20/2012 02:41 PM, Jiri Denemark wrote:
On Fri, Jan 20, 2012 at 14:06:26 -0700, Eric Blake wrote:
Although this is a public API break, it only affects users that were compiling against *_LAST values, and can be trivially worked around without impacting compilation against older headers, by the user defining LIBVIRT_ENUM_SENTINELS before including libvirt.h. It is not an ABI break, since enum values do not appear as .so entry points.
See this list discussion: https://www.redhat.com/archives/libvir-list/2012-January/msg00804.html
Overall, I like the patch but I don't agree with the way of silencing compilation warnings in the second part. The main benefit of avoiding default in switches used with enums is that the compiler is nice and warns us when new item is added to an enum. I'd prefer not to ruin this.
@@ -205,6 +205,7 @@ cpuTestCompResStr(virCPUCompareResult result) case VIR_CPU_COMPARE_INCOMPATIBLE: return "INCOMPATIBLE"; case VIR_CPU_COMPARE_IDENTICAL: return "IDENTICAL"; case VIR_CPU_COMPARE_SUPERSET: return "SUPERSET"; + default: ; case VIR_CPU_COMPARE_LAST: ;
Ah, using an explicit _LAST rather than a generic default: also silences the warning. Nice to know. Oh, and I just realized that I should have probably named it VIR_ENUM_SENTINELS, not LIBVIRT_ENUM_SENTINELS (which I had blindly copied from Daniel's email), to match the namespace that we used for all our other public macros; I did a quick search and replace to fix that before pushing.
ACK with those defaults removed.
Done and pushed, along with patch 2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Jan 20, 2012 at 10:41:37PM +0100, Jiri Denemark wrote:
On Fri, Jan 20, 2012 at 14:06:26 -0700, Eric Blake wrote:
Although this is a public API break, it only affects users that were compiling against *_LAST values, and can be trivially worked around without impacting compilation against older headers, by the user defining LIBVIRT_ENUM_SENTINELS before including libvirt.h. It is not an ABI break, since enum values do not appear as .so entry points.
See this list discussion: https://www.redhat.com/archives/libvir-list/2012-January/msg00804.html
* include/libvirt/libvirt.h.in: Hide all sentinels behind LIBVIRT_ENUM_SENTINELS, and add missing sentinels. * src/internal.h (VIR_DEPRECATED): Allow inclusion after libvirt.h. (LIBVIRT_ENUM_SENTINELS): Expose sentinels internally. * daemon/libvirtd.h: Use the sentinels. * src/remote/remote_protocol.x (includes): Don't expose sentinels. * python/generator.py (enum): Likewise. * tests/cputest.c (cpuTestCompResStr): Silence compiler warning. * tools/virsh.c (vshDomainStateReasonToString) (vshDomainControlStateToString): Likewise. --- daemon/libvirtd.h | 8 +- include/libvirt/libvirt.h.in | 186 ++++++++++++++++++++++++++++++++++++----- python/generator.py | 3 +- src/internal.h | 4 + src/remote/remote_protocol.x | 3 +- tests/cputest.c | 3 +- tools/virsh.c | 9 ++ 7 files changed, 187 insertions(+), 29 deletions(-)
Overall, I like the patch but I don't agree with the way of silencing compilation warnings in the second part. The main benefit of avoiding default in switches used with enums is that the compiler is nice and warns us when new item is added to an enum. I'd prefer not to ruin this.
Now that we have the _LAST members for these enums, we can kill off many of these crazy switch statements, and just use VIR_ENUM_IMPL which is statically checked. 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 :|

When converting a linear enum to a string, we have checks in place in the VIR_ENUM_IMPL macro to ensure that there is one string for every value, which lets us quickly flag if a user added a value but forgot to add a counterpart string. However, this only works if we use the _LAST marker. * cfg.mk (sc_require_enum_last_marker): New syntax check. * src/conf/domain_conf.h (virDomainSnapshotState): Add new marker. * src/conf/domain_conf.c (virDomainSnapshotState): Fix offender. * src/qemu/qemu_monitor_json.c (qemuMonitorWatchdogAction) (qemuMonitorIOErrorAction, qemuMonitorGraphicsAddressFamily): Likewise. * src/util/virtypedparam.c (virTypedParameter): Likewise. --- cfg.mk | 12 +++++++++++- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 3 ++- src/qemu/qemu_monitor_json.c | 9 +++++---- src/util/virtypedparam.c | 2 +- 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/cfg.mk b/cfg.mk index 817b5f3..95ba61c 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1,5 +1,5 @@ # Customize Makefile.maint. -*- makefile -*- -# Copyright (C) 2008-2011 Red Hat, Inc. +# Copyright (C) 2008-2012 Red Hat, Inc. # Copyright (C) 2003-2008 Free Software Foundation, Inc. # This program is free software: you can redistribute it and/or modify @@ -610,6 +610,16 @@ sc_prohibit_gettext_markup: halt='do not mark these strings for translation' \ $(_sc_search_regexp) +# When converting an enum to a string, make sure that we track any new +# elements added to the enum by using a _LAST marker. +sc_require_enum_last_marker: + @grep -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \ + | sed -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ + -e '/VIR_ENUM_IMPL[^,]*,[^,]*[^_][^L][^A][^S][^T],/p' \ + | grep . && \ + { echo '$(ME): enum impl needs to use _LAST marker' 1>&2; \ + exit 1; } || : + # We don't use this feature of maint.mk. prev_version_file = /dev/null diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f97014e..8eed85b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -500,7 +500,7 @@ VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_LAST, "crashed") /* virDomainSnapshotState is really virDomainState plus one extra state */ -VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_DISK_SNAPSHOT+1, +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST, "nostate", "running", "blocked", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b121f9c..a49795c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1,7 +1,7 @@ /* * domain_conf.h: domain XML processing * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -328,6 +328,7 @@ enum virDomainDiskSnapshot { enum virDomainSnapshotState { /* Inherit the VIR_DOMAIN_* states from virDomainState. */ VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, + VIR_DOMAIN_SNAPSHOT_STATE_LAST }; enum virDomainStartupPolicy { diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7eb2a92..4a76fc0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1,7 +1,7 @@ /* * qemu_monitor_json.c: interaction with QEMU monitor console * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -554,7 +554,7 @@ static void qemuMonitorJSONHandleRTCChange(qemuMonitorPtr mon, virJSONValuePtr d } VIR_ENUM_DECL(qemuMonitorWatchdogAction) -VIR_ENUM_IMPL(qemuMonitorWatchdogAction, VIR_DOMAIN_EVENT_WATCHDOG_DEBUG + 1, +VIR_ENUM_IMPL(qemuMonitorWatchdogAction, VIR_DOMAIN_EVENT_WATCHDOG_LAST, "none", "pause", "reset", "poweroff", "shutdown", "debug"); static void qemuMonitorJSONHandleWatchdog(qemuMonitorPtr mon, virJSONValuePtr data) @@ -576,7 +576,7 @@ static void qemuMonitorJSONHandleWatchdog(qemuMonitorPtr mon, virJSONValuePtr da } VIR_ENUM_DECL(qemuMonitorIOErrorAction) -VIR_ENUM_IMPL(qemuMonitorIOErrorAction, VIR_DOMAIN_EVENT_IO_ERROR_REPORT + 1, +VIR_ENUM_IMPL(qemuMonitorIOErrorAction, VIR_DOMAIN_EVENT_IO_ERROR_LAST, "ignore", "stop", "report"); @@ -619,7 +619,8 @@ static void qemuMonitorJSONHandleIOError(qemuMonitorPtr mon, virJSONValuePtr dat VIR_ENUM_DECL(qemuMonitorGraphicsAddressFamily) -VIR_ENUM_IMPL(qemuMonitorGraphicsAddressFamily, VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_UNIX + 1, +VIR_ENUM_IMPL(qemuMonitorGraphicsAddressFamily, + VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_LAST, "ipv4", "ipv6", "unix"); static void qemuMonitorJSONHandleVNC(qemuMonitorPtr mon, virJSONValuePtr data, int phase) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index f71aa25..48ee5d5 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -35,7 +35,7 @@ __FUNCTION__, __LINE__, __VA_ARGS__) VIR_ENUM_DECL(virTypedParameter) -VIR_ENUM_IMPL(virTypedParameter, VIR_TYPED_PARAM_STRING + 1, +VIR_ENUM_IMPL(virTypedParameter, VIR_TYPED_PARAM_LAST, "unknown", "int", "uint", -- 1.7.7.5

On Fri, Jan 20, 2012 at 14:06:27 -0700, Eric Blake wrote:
When converting a linear enum to a string, we have checks in place in the VIR_ENUM_IMPL macro to ensure that there is one string for every value, which lets us quickly flag if a user added a value but forgot to add a counterpart string. However, this only works if we use the _LAST marker.
* cfg.mk (sc_require_enum_last_marker): New syntax check. * src/conf/domain_conf.h (virDomainSnapshotState): Add new marker. * src/conf/domain_conf.c (virDomainSnapshotState): Fix offender. * src/qemu/qemu_monitor_json.c (qemuMonitorWatchdogAction) (qemuMonitorIOErrorAction, qemuMonitorGraphicsAddressFamily): Likewise. * src/util/virtypedparam.c (virTypedParameter): Likewise.
ACK Jirka
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Jiri Denemark