[libvirt] [PATCH] conf: avoid NULL deref for pmsuspended domain state
While working with a pmsuspend vs. snapshot issue, I noticed that the state file in /var/run/libvirt/qemu/dom.xml contained a rather suspicious "(null)" string, which does not round-trip well through a libvirtd restart. Had I been on a platform other than glibc where printf("%s",NULL) crashes instead of printing (null), we might have noticed the problem much sooner. And in fixing that problem, I also noticed that we had several missing states, because we were #defining several *_LAST names to a value _different_ than what they were already given as enums in libvirt.h. Yuck. I got rid of default: labels in the case statements, because they get in the way of gcc's -Wswitch helping us ensure we cover all enum values. * src/conf/domain_conf.c (virDomainStateReasonToString) (virDomainStateReasonFromString): Fill in missing domain states; rewrite case statement to let compiler enforce checking. (VIR_DOMAIN_NOSTATE_LAST, VIR_DOMAIN_RUNNING_LAST) (VIR_DOMAIN_BLOCKED_LAST, VIR_DOMAIN_PAUSED_LAST) (VIR_DOMAIN_SHUTDOWN_LAST, VIR_DOMAIN_SHUTOFF_LAST) (VIR_DOMAIN_CRASHED_LAST): Drop dead defines. (VIR_DOMAIN_PMSUSPENDED_LAST): Drop dead define. (virDomainPMSuspendedReason): Add missing enum function. (virDomainRunningReason, virDomainPausedReason): Add missing enum value. * src/conf/domain_conf.h (virDomainPMSuspendedReason): Declare missing functions. * src/libvirt_private.syms (domain_conf.h): Export them. --- src/conf/domain_conf.c | 32 +++++++++++++++++++------------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 6 ++++-- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7273790..5782abb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -574,11 +574,9 @@ VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_LAST, "crashed", "pmsuspended") -#define VIR_DOMAIN_NOSTATE_LAST (VIR_DOMAIN_NOSTATE_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainNostateReason, VIR_DOMAIN_NOSTATE_LAST, "unknown") -#define VIR_DOMAIN_RUNNING_LAST (VIR_DOMAIN_RUNNING_SAVE_CANCELED + 1) VIR_ENUM_IMPL(virDomainRunningReason, VIR_DOMAIN_RUNNING_LAST, "unknown", "booted", @@ -587,13 +585,12 @@ VIR_ENUM_IMPL(virDomainRunningReason, VIR_DOMAIN_RUNNING_LAST, "from snapshot", "unpaused", "migration canceled", - "save canceled") + "save canceled", + "wakeup") -#define VIR_DOMAIN_BLOCKED_LAST (VIR_DOMAIN_BLOCKED_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainBlockedReason, VIR_DOMAIN_BLOCKED_LAST, "unknown") -#define VIR_DOMAIN_PAUSED_LAST (VIR_DOMAIN_PAUSED_SHUTTING_DOWN + 1) VIR_ENUM_IMPL(virDomainPausedReason, VIR_DOMAIN_PAUSED_LAST, "unknown", "user", @@ -603,14 +600,13 @@ VIR_ENUM_IMPL(virDomainPausedReason, VIR_DOMAIN_PAUSED_LAST, "ioerror", "watchdog", "from snapshot", - "shutdown") + "shutdown", + "snapshot") -#define VIR_DOMAIN_SHUTDOWN_LAST (VIR_DOMAIN_SHUTDOWN_USER + 1) VIR_ENUM_IMPL(virDomainShutdownReason, VIR_DOMAIN_SHUTDOWN_LAST, "unknown", "user") -#define VIR_DOMAIN_SHUTOFF_LAST (VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT + 1) VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST, "unknown", "shutdown", @@ -621,10 +617,12 @@ VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST, "failed", "from snapshot") -#define VIR_DOMAIN_CRASHED_LAST (VIR_DOMAIN_CRASHED_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST, "unknown") +VIR_ENUM_IMPL(virDomainPMSuspendedReason, VIR_DOMAIN_PMSUSPENDED_LAST, + "unknown") + VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, "default", "none", @@ -15438,9 +15436,13 @@ virDomainStateReasonToString(virDomainState state, int reason) return virDomainShutoffReasonTypeToString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeToString(reason); - default: - return NULL; + case VIR_DOMAIN_PMSUSPENDED: + return virDomainPMSuspendedReasonTypeToString(reason); + case VIR_DOMAIN_LAST: + break; } + VIR_WARN("Unexpected domain state: %d", state); + return NULL; } @@ -15462,9 +15464,13 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) return virDomainShutoffReasonTypeFromString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeFromString(reason); - default: - return -1; + case VIR_DOMAIN_PMSUSPENDED: + return virDomainPMSuspendedReasonTypeFromString(reason); + case VIR_DOMAIN_LAST: + break; } + VIR_WARN("Unexpected domain state: %d", state); + return -1; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index abbfe86..9a9e0b1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2315,6 +2315,7 @@ VIR_ENUM_DECL(virDomainPausedReason) VIR_ENUM_DECL(virDomainShutdownReason) VIR_ENUM_DECL(virDomainShutoffReason) VIR_ENUM_DECL(virDomainCrashedReason) +VIR_ENUM_DECL(virDomainPMSuspendedReason) const char *virDomainStateReasonToString(virDomainState state, int reason); int virDomainStateReasonFromString(virDomainState state, const char *reason); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc23adc..57ecc36 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -487,10 +487,12 @@ virDomainObjSetState; virDomainObjTaint; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; -virDomainPciRombarModeTypeFromString; -virDomainPciRombarModeTypeToString; virDomainPMStateTypeFromString; virDomainPMStateTypeToString; +virDomainPMSuspendedReasonTypeFromString; +virDomainPMSuspendedReasonTypeToString; +virDomainPciRombarModeTypeFromString; +virDomainPciRombarModeTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRemoveInactive; -- 1.8.1
I got bit by 'make check' complaining that the sort order I got by emacs' sort-lines function differed from expectations. * src/libvirt_private.syms: Add emacs trailer. --- Okay to push this as well? My previous patch mistakenly did this:
+++ b/src/libvirt_private.syms @@ -487,10 +487,12 @@ virDomainObjSetState; virDomainObjTaint; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; -virDomainPciRombarModeTypeFromString; -virDomainPciRombarModeTypeToString; virDomainPMStateTypeFromString; virDomainPMStateTypeToString; +virDomainPMSuspendedReasonTypeFromString; +virDomainPMSuspendedReasonTypeToString; +virDomainPciRombarModeTypeFromString; +virDomainPciRombarModeTypeToString; virDomainRedirdevBusTypeFromString;
I didn't catch that until 'make check' after I had already posted; I've fixed that sorting locally, but this patch would let me avoid the same problem in the future. src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f0e4cf..db2ff62 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1910,3 +1910,8 @@ virXPathUInt; virXPathULong; virXPathULongHex; virXPathULongLong; + +# Let emacs know we want case-insensitive sorting +# Local Variables: +# sort-fold-case: t +# End: -- 1.8.1
On Wed, Jan 23, 2013 at 06:23:56PM -0700, Eric Blake wrote:
I got bit by 'make check' complaining that the sort order I got by emacs' sort-lines function differed from expectations.
* src/libvirt_private.syms: Add emacs trailer. ---
Okay to push this as well? My previous patch mistakenly did this:
+++ b/src/libvirt_private.syms @@ -487,10 +487,12 @@ virDomainObjSetState; virDomainObjTaint; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; -virDomainPciRombarModeTypeFromString; -virDomainPciRombarModeTypeToString; virDomainPMStateTypeFromString; virDomainPMStateTypeToString; +virDomainPMSuspendedReasonTypeFromString; +virDomainPMSuspendedReasonTypeToString; +virDomainPciRombarModeTypeFromString; +virDomainPciRombarModeTypeToString; virDomainRedirdevBusTypeFromString;
I didn't catch that until 'make check' after I had already posted; I've fixed that sorting locally, but this patch would let me avoid the same problem in the future.
src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1f0e4cf..db2ff62 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1910,3 +1910,8 @@ virXPathUInt; virXPathULong; virXPathULongHex; virXPathULongLong; + +# Let emacs know we want case-insensitive sorting +# Local Variables: +# sort-fold-case: t +# End: --
ACK 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 2013年01月24日 08:12, Eric Blake wrote:
While working with a pmsuspend vs. snapshot issue, I noticed that the state file in /var/run/libvirt/qemu/dom.xml contained a rather suspicious "(null)" string, which does not round-trip well through a libvirtd restart. Had I been on a platform other than glibc where printf("%s",NULL) crashes instead of printing (null), we might have noticed the problem much sooner.
And in fixing that problem, I also noticed that we had several missing states, because we were #defining several *_LAST names to a value _different_ than what they were already given as enums in libvirt.h. Yuck. I got rid of default: labels in the case statements, because they get in the way of gcc's -Wswitch helping us ensure we cover all enum values.
* src/conf/domain_conf.c (virDomainStateReasonToString) (virDomainStateReasonFromString): Fill in missing domain states; rewrite case statement to let compiler enforce checking. (VIR_DOMAIN_NOSTATE_LAST, VIR_DOMAIN_RUNNING_LAST) (VIR_DOMAIN_BLOCKED_LAST, VIR_DOMAIN_PAUSED_LAST) (VIR_DOMAIN_SHUTDOWN_LAST, VIR_DOMAIN_SHUTOFF_LAST) (VIR_DOMAIN_CRASHED_LAST): Drop dead defines. (VIR_DOMAIN_PMSUSPENDED_LAST): Drop dead define. (virDomainPMSuspendedReason): Add missing enum function. (virDomainRunningReason, virDomainPausedReason): Add missing enum value. * src/conf/domain_conf.h (virDomainPMSuspendedReason): Declare missing functions. * src/libvirt_private.syms (domain_conf.h): Export them. --- src/conf/domain_conf.c | 32 +++++++++++++++++++------------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 6 ++++-- 3 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7273790..5782abb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -574,11 +574,9 @@ VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_LAST, "crashed", "pmsuspended")
-#define VIR_DOMAIN_NOSTATE_LAST (VIR_DOMAIN_NOSTATE_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainNostateReason, VIR_DOMAIN_NOSTATE_LAST, "unknown")
-#define VIR_DOMAIN_RUNNING_LAST (VIR_DOMAIN_RUNNING_SAVE_CANCELED + 1) VIR_ENUM_IMPL(virDomainRunningReason, VIR_DOMAIN_RUNNING_LAST, "unknown", "booted", @@ -587,13 +585,12 @@ VIR_ENUM_IMPL(virDomainRunningReason, VIR_DOMAIN_RUNNING_LAST, "from snapshot", "unpaused", "migration canceled", - "save canceled") + "save canceled", + "wakeup")
-#define VIR_DOMAIN_BLOCKED_LAST (VIR_DOMAIN_BLOCKED_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainBlockedReason, VIR_DOMAIN_BLOCKED_LAST, "unknown")
-#define VIR_DOMAIN_PAUSED_LAST (VIR_DOMAIN_PAUSED_SHUTTING_DOWN + 1) VIR_ENUM_IMPL(virDomainPausedReason, VIR_DOMAIN_PAUSED_LAST, "unknown", "user", @@ -603,14 +600,13 @@ VIR_ENUM_IMPL(virDomainPausedReason, VIR_DOMAIN_PAUSED_LAST, "ioerror", "watchdog", "from snapshot", - "shutdown") + "shutdown", + "snapshot")
-#define VIR_DOMAIN_SHUTDOWN_LAST (VIR_DOMAIN_SHUTDOWN_USER + 1) VIR_ENUM_IMPL(virDomainShutdownReason, VIR_DOMAIN_SHUTDOWN_LAST, "unknown", "user")
-#define VIR_DOMAIN_SHUTOFF_LAST (VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT + 1) VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST, "unknown", "shutdown", @@ -621,10 +617,12 @@ VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST, "failed", "from snapshot")
-#define VIR_DOMAIN_CRASHED_LAST (VIR_DOMAIN_CRASHED_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST, "unknown")
+VIR_ENUM_IMPL(virDomainPMSuspendedReason, VIR_DOMAIN_PMSUSPENDED_LAST, + "unknown") + VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, "default", "none", @@ -15438,9 +15436,13 @@ virDomainStateReasonToString(virDomainState state, int reason) return virDomainShutoffReasonTypeToString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeToString(reason); - default: - return NULL; + case VIR_DOMAIN_PMSUSPENDED: + return virDomainPMSuspendedReasonTypeToString(reason); + case VIR_DOMAIN_LAST: + break; } + VIR_WARN("Unexpected domain state: %d", state); + return NULL; }
@@ -15462,9 +15464,13 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) return virDomainShutoffReasonTypeFromString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeFromString(reason); - default: - return -1; + case VIR_DOMAIN_PMSUSPENDED: + return virDomainPMSuspendedReasonTypeFromString(reason); + case VIR_DOMAIN_LAST: + break; } + VIR_WARN("Unexpected domain state: %d", state); + return -1; }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index abbfe86..9a9e0b1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2315,6 +2315,7 @@ VIR_ENUM_DECL(virDomainPausedReason) VIR_ENUM_DECL(virDomainShutdownReason) VIR_ENUM_DECL(virDomainShutoffReason) VIR_ENUM_DECL(virDomainCrashedReason) +VIR_ENUM_DECL(virDomainPMSuspendedReason)
const char *virDomainStateReasonToString(virDomainState state, int reason); int virDomainStateReasonFromString(virDomainState state, const char *reason); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc23adc..57ecc36 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -487,10 +487,12 @@ virDomainObjSetState; virDomainObjTaint; virDomainPausedReasonTypeFromString; virDomainPausedReasonTypeToString; -virDomainPciRombarModeTypeFromString; -virDomainPciRombarModeTypeToString; virDomainPMStateTypeFromString; virDomainPMStateTypeToString; +virDomainPMSuspendedReasonTypeFromString; +virDomainPMSuspendedReasonTypeToString; +virDomainPciRombarModeTypeFromString; +virDomainPciRombarModeTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRemoveInactive;
ACK, with the emacs tail. But it will be nice to use the emacs tail for all the private syms files before pushing. Osier
On 01/23/2013 07:55 PM, Osier Yang wrote:
On 2013年01月24日 08:12, Eric Blake wrote:
While working with a pmsuspend vs. snapshot issue, I noticed that the state file in /var/run/libvirt/qemu/dom.xml contained a rather suspicious "(null)" string, which does not round-trip well through a libvirtd restart. Had I been on a platform other than glibc where printf("%s",NULL) crashes instead of printing (null), we might have noticed the problem much sooner.
And in fixing that problem, I also noticed that we had several missing states, because we were #defining several *_LAST names to a value _different_ than what they were already given as enums in libvirt.h. Yuck. I got rid of default: labels in the case statements, because they get in the way of gcc's -Wswitch helping us ensure we cover all enum values.
ACK, with the emacs tail. But it will be nice to use the emacs tail for all the private syms files before pushing.
Sure, I reordered the patch, and added the emacs tail to ALL the syms files, before pushing. Thanks for the review. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang