8:08 p.m.
The use of static storage to append ": %s" is not thread-safe; another
thread colling virErrorMsg at the same time will clobber us. However,
this condition is temporary, since a future patch will inline this
lookup directly into virRaiseErrorFull.
* src/util/virterror.c (virErrorMsg): Make appending ": %s"
uniform, to make it easier to spot remaining odd formats.
Remove unused VIR_ERR_CALL_FAILED, and add a default case.
* tests/cpuset: Adjust expected output.
* tests/undefine: Likewise.
---
I could make this thread-safe, but the goal is to eventually collapse
the entire switch statement into an array-based lookup inlined into
virRaiseErrorFull, at which point, the allocation and copying into
thread-safe storage can again be threadsafe.
Even if this gets ACK'd now, I won't push until I complete the series
and have the inlining complete back into a thread-safe solution. The
remaining patches in the current series knock out some of the FIXMEs
documented in this patch, and the concept of fixing them is relatively
easy for the remaining unwritten patches, taking mostly the
time-consuming effort to audit each use.
src/util/virterror.c | 338 ++++++++++++++++++--------------------------------
tests/cpuset | 4 +-
tests/undefine | 4 +-
3 files changed, 127 insertions(+), 219 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c
index 96dd1e7..f442fae 100644
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -739,437 +739,345 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED,
* Internal routine to get the message associated to an error raised
* from the library
*
- * Returns the constant string associated to @error
+ * Returns the constant string associated to @error, or NULL if @error
+ * is VIR_ERR_OK. The string is only valid until the next call to
+ * virErrorMsg().
*/
const char *
virErrorMsg(virErrorNumber error, const char *info)
{
const char *errmsg = NULL;
+ static char *buffer; /* FIXME: Not thread-safe. */
+ static size_t buffer_length;
switch (error) {
case VIR_ERR_OK:
return (NULL);
case VIR_ERR_INTERNAL_ERROR:
- if (info != NULL)
- errmsg = _("internal error %s");
- else
- errmsg = _("internal error");
+ errmsg = _("internal error");
break;
case VIR_ERR_NO_MEMORY:
errmsg = _("out of memory");
break;
case VIR_ERR_NO_SUPPORT:
- if (info == NULL)
- errmsg = _("this function is not supported by the
hypervisor");
- else
- errmsg = _("this function is not supported by the hypervisor:
%s");
+ errmsg = _("this function is not supported by the hypervisor");
break;
case VIR_ERR_NO_CONNECT:
- if (info == NULL)
- errmsg = _("no hypervisor driver available");
- else
- errmsg = _("no hypervisor driver available for %s");
+ errmsg = _("no hypervisor driver available");
break;
case VIR_ERR_INVALID_CONN:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("invalid connection pointer in");
else
errmsg = _("invalid connection pointer in %s");
+ info = NULL;
break;
case VIR_ERR_INVALID_DOMAIN:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("invalid domain pointer in");
else
errmsg = _("invalid domain pointer in %s");
+ info = NULL;
break;
case VIR_ERR_INVALID_ARG:
+ /* FIXME: Adjust all callers before unifying semantics. */
+ info = NULL;
if (info == NULL)
errmsg = _("invalid argument in");
else
errmsg = _("invalid argument in %s");
+ info = NULL;
break;
case VIR_ERR_OPERATION_FAILED:
- if (info != NULL)
- errmsg = _("operation failed: %s");
- else
- errmsg = _("operation failed");
+ errmsg = _("operation failed");
break;
case VIR_ERR_GET_FAILED:
- if (info != NULL)
- errmsg = _("GET operation failed: %s");
- else
- errmsg = _("GET operation failed");
+ errmsg = _("GET operation failed");
break;
case VIR_ERR_POST_FAILED:
- if (info != NULL)
- errmsg = _("POST operation failed: %s");
- else
- errmsg = _("POST operation failed");
+ errmsg = _("POST operation failed");
break;
case VIR_ERR_HTTP_ERROR:
+ /* FIXME: Adjust all callers before unifying semantics. */
errmsg = _("got unknown HTTP error code %d");
+ info = NULL;
break;
case VIR_ERR_UNKNOWN_HOST:
- if (info != NULL)
- errmsg = _("unknown host %s");
- else
- errmsg = _("unknown host");
+ errmsg = _("unknown host");
break;
case VIR_ERR_SEXPR_SERIAL:
- if (info != NULL)
- errmsg = _("failed to serialize S-Expr: %s");
- else
- errmsg = _("failed to serialize S-Expr");
+ errmsg = _("failed to serialize S-Expr");
break;
case VIR_ERR_NO_XEN:
- if (info == NULL)
- errmsg = _("could not use Xen hypervisor entry");
- else
- errmsg = _("could not use Xen hypervisor entry %s");
+ errmsg = _("could not use Xen hypervisor entry");
break;
case VIR_ERR_NO_XENSTORE:
- if (info == NULL)
- errmsg = _("could not connect to Xen Store");
- else
- errmsg = _("could not connect to Xen Store %s");
+ errmsg = _("could not connect to Xen Store");
break;
case VIR_ERR_XEN_CALL:
+ /* FIXME: Adjust all callers before unifying semantics. */
errmsg = _("failed Xen syscall %s");
break;
case VIR_ERR_OS_TYPE:
- if (info == NULL)
- errmsg = _("unknown OS type");
- else
- errmsg = _("unknown OS type %s");
+ errmsg = _("unknown OS type");
break;
case VIR_ERR_NO_KERNEL:
errmsg = _("missing kernel information");
break;
case VIR_ERR_NO_ROOT:
- if (info == NULL)
- errmsg = _("missing root device information");
- else
- errmsg = _("missing root device information in %s");
+ errmsg = _("missing root device information");
break;
case VIR_ERR_NO_SOURCE:
- if (info == NULL)
- errmsg = _("missing source information for device");
- else
- errmsg = _("missing source information for device %s");
+ errmsg = _("missing source information for device");
break;
case VIR_ERR_NO_TARGET:
- if (info == NULL)
- errmsg = _("missing target information for device");
- else
- errmsg = _("missing target information for device %s");
+ errmsg = _("missing target information for device");
break;
case VIR_ERR_NO_NAME:
- if (info == NULL)
- errmsg = _("missing domain name information");
- else
- errmsg = _("missing domain name information in %s");
+ errmsg = _("missing domain name information");
break;
case VIR_ERR_NO_OS:
- if (info == NULL)
- errmsg = _("missing operating system information");
- else
- errmsg = _("missing operating system information for %s");
+ errmsg = _("missing operating system information");
break;
case VIR_ERR_NO_DEVICE:
- if (info == NULL)
- errmsg = _("missing devices information");
- else
- errmsg = _("missing devices information for %s");
+ errmsg = _("missing device information");
break;
case VIR_ERR_DRIVER_FULL:
- if (info == NULL)
- errmsg = _("too many drivers registered");
- else
- errmsg = _("too many drivers registered in %s");
- break;
- case VIR_ERR_CALL_FAILED: /* DEPRECATED, use VIR_ERR_NO_SUPPORT */
- if (info == NULL)
- errmsg = _("library call failed, possibly not supported");
- else
- errmsg = _("library call %s failed, possibly not supported");
+ errmsg = _("too many drivers registered");
break;
case VIR_ERR_XML_ERROR:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("XML description not well formed or invalid");
else
errmsg = _("XML description for %s is not well formed or
invalid");
+ info = NULL;
break;
case VIR_ERR_DOM_EXIST:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("this domain exists already");
else
errmsg = _("domain %s exists already");
+ info = NULL;
break;
case VIR_ERR_OPERATION_DENIED:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("operation forbidden for read only access");
else
errmsg = _("operation %s forbidden for read only access");
+ info = NULL;
break;
case VIR_ERR_OPEN_FAILED:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("failed to open configuration file for reading");
else
errmsg = _("failed to open %s for reading");
+ info = NULL;
break;
case VIR_ERR_READ_FAILED:
- if (info == NULL)
- errmsg = _("failed to read configuration file");
- else
- errmsg = _("failed to read configuration file %s");
+ errmsg = _("failed to read configuration file");
break;
case VIR_ERR_PARSE_FAILED:
- if (info == NULL)
- errmsg = _("failed to parse configuration file");
- else
- errmsg = _("failed to parse configuration file %s");
+ errmsg = _("failed to parse configuration file");
break;
case VIR_ERR_CONF_SYNTAX:
- if (info == NULL)
- errmsg = _("configuration file syntax error");
- else
- errmsg = _("configuration file syntax error: %s");
+ errmsg = _("configuration file syntax error");
break;
case VIR_ERR_WRITE_FAILED:
- if (info == NULL)
- errmsg = _("failed to write configuration file");
- else
- errmsg = _("failed to write configuration file: %s");
+ errmsg = _("failed to write configuration file");
break;
case VIR_ERR_XML_DETAIL:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("parser error");
else
errmsg = "%s";
+ info = NULL;
break;
case VIR_ERR_INVALID_NETWORK:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("invalid network pointer in");
else
errmsg = _("invalid network pointer in %s");
+ info = NULL;
break;
case VIR_ERR_NETWORK_EXIST:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("this network exists already");
else
errmsg = _("network %s exists already");
+ info = NULL;
break;
case VIR_ERR_SYSTEM_ERROR:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("system call error");
else
errmsg = "%s";
+ info = NULL;
break;
case VIR_ERR_RPC:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("RPC error");
else
errmsg = "%s";
+ info = NULL;
break;
case VIR_ERR_GNUTLS_ERROR:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("GNUTLS call error");
else
errmsg = "%s";
+ info = NULL;
break;
case VIR_WAR_NO_NETWORK:
- if (info == NULL)
- errmsg = _("Failed to find the network");
- else
- errmsg = _("Failed to find the network: %s");
+ errmsg = _("Failed to find the network");
break;
case VIR_ERR_NO_DOMAIN:
- if (info == NULL)
- errmsg = _("Domain not found");
- else
- errmsg = _("Domain not found: %s");
+ errmsg = _("Domain not found");
break;
case VIR_ERR_NO_NETWORK:
- if (info == NULL)
- errmsg = _("Network not found");
- else
- errmsg = _("Network not found: %s");
+ errmsg = _("Network not found");
break;
case VIR_ERR_INVALID_MAC:
- if (info == NULL)
- errmsg = _("invalid MAC address");
- else
- errmsg = _("invalid MAC address: %s");
+ errmsg = _("invalid MAC address");
break;
case VIR_ERR_AUTH_FAILED:
- if (info == NULL)
- errmsg = _("authentication failed");
- else
- errmsg = _("authentication failed: %s");
+ errmsg = _("authentication failed");
break;
case VIR_ERR_NO_STORAGE_POOL:
- if (info == NULL)
- errmsg = _("Storage pool not found");
- else
- errmsg = _("Storage pool not found: %s");
+ errmsg = _("Storage pool not found");
break;
case VIR_ERR_NO_STORAGE_VOL:
- if (info == NULL)
- errmsg = _("Storage volume not found");
- else
- errmsg = _("Storage volume not found: %s");
+ errmsg = _("Storage volume not found");
break;
case VIR_ERR_INVALID_STORAGE_POOL:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
- errmsg = _("invalid storage pool pointer in");
+ errmsg = _("invalid storage pool pointer in");
else
- errmsg = _("invalid storage pool pointer in %s");
+ errmsg = _("invalid storage pool pointer in %s");
+ info = NULL;
break;
case VIR_ERR_INVALID_STORAGE_VOL:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
- errmsg = _("invalid storage volume pointer in");
+ errmsg = _("invalid storage volume pointer in");
else
- errmsg = _("invalid storage volume pointer in %s");
+ errmsg = _("invalid storage volume pointer in %s");
+ info = NULL;
break;
case VIR_WAR_NO_STORAGE:
- if (info == NULL)
- errmsg = _("Failed to find a storage driver");
- else
- errmsg = _("Failed to find a storage driver: %s");
+ errmsg = _("Failed to find a storage driver");
break;
case VIR_WAR_NO_NODE:
- if (info == NULL)
- errmsg = _("Failed to find a node driver");
- else
- errmsg = _("Failed to find a node driver: %s");
+ errmsg = _("Failed to find a node driver");
break;
case VIR_ERR_INVALID_NODE_DEVICE:
- if (info == NULL)
- errmsg = _("invalid node device pointer");
- else
- errmsg = _("invalid node device pointer in %s");
+ errmsg = _("invalid node device pointer");
break;
case VIR_ERR_NO_NODE_DEVICE:
- if (info == NULL)
- errmsg = _("Node device not found");
- else
- errmsg = _("Node device not found: %s");
+ errmsg = _("Node device not found");
break;
case VIR_ERR_NO_SECURITY_MODEL:
- if (info == NULL)
- errmsg = _("Security model not found");
- else
- errmsg = _("Security model not found: %s");
+ errmsg = _("Security model not found");
break;
case VIR_ERR_OPERATION_INVALID:
- if (info == NULL)
- errmsg = _("Requested operation is not valid");
- else
- errmsg = _("Requested operation is not valid: %s");
+ errmsg = _("Requested operation is not valid");
break;
case VIR_WAR_NO_INTERFACE:
- if (info == NULL)
- errmsg = _("Failed to find the interface");
- else
- errmsg = _("Failed to find the interface: %s");
+ errmsg = _("Failed to find the interface");
break;
case VIR_ERR_NO_INTERFACE:
- if (info == NULL)
- errmsg = _("Interface not found");
- else
- errmsg = _("Interface not found: %s");
+ errmsg = _("Interface not found");
break;
case VIR_ERR_INVALID_INTERFACE:
+ /* FIXME: Adjust all callers before unifying semantics. */
if (info == NULL)
errmsg = _("invalid interface pointer in");
else
errmsg = _("invalid interface pointer in %s");
+ info = NULL;
break;
case VIR_ERR_MULTIPLE_INTERFACES:
- if (info == NULL)
- errmsg = _("multiple matching interfaces found");
- else
- errmsg = _("multiple matching interfaces found: %s");
+ errmsg = _("multiple matching interfaces found");
break;
case VIR_WAR_NO_SECRET:
- if (info == NULL)
- errmsg = _("Failed to find a secret storage driver");
- else
- errmsg = _("Failed to find a secret storage driver: %s");
+ errmsg = _("Failed to find a secret storage driver");
break;
case VIR_ERR_INVALID_SECRET:
- if (info == NULL)
- errmsg = _("Invalid secret");
- else
- errmsg = _("Invalid secret: %s");
+ errmsg = _("Invalid secret");
break;
case VIR_ERR_NO_SECRET:
- if (info == NULL)
- errmsg = _("Secret not found");
- else
- errmsg = _("Secret not found: %s");
+ errmsg = _("Secret not found");
break;
case VIR_WAR_NO_NWFILTER:
- if (info == NULL)
- errmsg = _("Failed to start the nwfilter driver");
- else
- errmsg = _("Failed to start the nwfilter driver: %s");
+ errmsg = _("Failed to start the nwfilter driver");
break;
case VIR_ERR_INVALID_NWFILTER:
- if (info == NULL)
- errmsg = _("Invalid network filter");
- else
- errmsg = _("Invalid network filter: %s");
+ errmsg = _("Invalid network filter");
break;
case VIR_ERR_NO_NWFILTER:
- if (info == NULL)
- errmsg = _("Network filter not found");
- else
- errmsg = _("Network filter not found: %s");
+ errmsg = _("Network filter not found");
break;
case VIR_ERR_BUILD_FIREWALL:
- if (info == NULL)
- errmsg = _("Error while building firewall");
- else
- errmsg = _("Error while building firewall: %s");
+ errmsg = _("Error while building firewall");
break;
case VIR_ERR_CONFIG_UNSUPPORTED:
- if (info == NULL)
- errmsg = _("unsupported configuration");
- else
- errmsg = _("unsupported configuration: %s");
+ errmsg = _("unsupported configuration");
break;
case VIR_ERR_OPERATION_TIMEOUT:
- if (info == NULL)
- errmsg = _("Timed out during operation");
- else
- errmsg = _("Timed out during operation: %s");
+ errmsg = _("Timed out during operation");
break;
case VIR_ERR_MIGRATE_PERSIST_FAILED:
- if (info == NULL)
- errmsg = _("Failed to make domain persistent after
migration");
- else
- errmsg = _("Failed to make domain persistent after migration:
%s");
+ errmsg = _("Failed to make domain persistent after migration");
break;
case VIR_ERR_HOOK_SCRIPT_FAILED:
- if (info == NULL)
- errmsg = _("Hook script execution failed");
- else
- errmsg = _("Hook script execution failed: %s");
+ errmsg = _("Hook script execution failed");
break;
case VIR_ERR_INVALID_DOMAIN_SNAPSHOT:
- if (info == NULL)
- errmsg = _("Invalid snapshot");
- else
- errmsg = _("Invalid snapshot: %s");
+ errmsg = _("Invalid snapshot");
break;
case VIR_ERR_NO_DOMAIN_SNAPSHOT:
- if (info == NULL)
- errmsg = _("Domain snapshot not found");
- else
- errmsg = _("Domain snapshot not found: %s");
+ errmsg = _("Domain snapshot not found");
break;
+
+ case VIR_ERR_CALL_FAILED: /* DEPRECATED, use VIR_ERR_NO_SUPPORT */
+ default:
+ errmsg = _("unexpected error code");
+ info = NULL;
+ break;
+ }
+
+ if (info) {
+ if (buffer_length < strlen(errmsg) + 5) {
+ size_t newlen = strlen(errmsg) + 5; /* strlen(": %s"), NUL */
+ char *newptr;
+
+ if (newlen < buffer_length * 2)
+ newlen = buffer_length * 2;
+ newptr = realloc(buffer, newlen);
+ if (!newptr) {
+ /* Out of memory while reporting an error :(
+ Best-effort will have to suffice. */
+ return errmsg;
+ }
+ buffer = newptr;
+ buffer_length = newlen;
+ }
+ snprintf(buffer, buffer_length, "%s: %%s", errmsg);
+ errmsg = buffer;
}
+
return (errmsg);
}
diff --git a/tests/cpuset b/tests/cpuset
index 800d3bc..0e299f1 100755
--- a/tests/cpuset
+++ b/tests/cpuset
@@ -1,7 +1,7 @@
#!/bin/sh
# ensure that defining with an invalid vCPU cpuset elicits a diagnostic
-# Copyright (C) 2008-2009 Red Hat, Inc.
+# Copyright (C) 2008-2010 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -41,7 +41,7 @@ sed "s/vcpu>/vcpu cpuset='aaa'>/" xml >
xml-invalid || fail=1
$abs_top_builddir/tools/virsh --connect test:///default define xml-invalid > out
2>&1 && fail=1
cat <<\EOF > exp || fail=1
error: Failed to define domain from xml-invalid
-error: internal error topology cpuset syntax error
+error: internal error: topology cpuset syntax error
EOF
compare exp out || fail=1
diff --git a/tests/undefine b/tests/undefine
index f89a91e..49a0de0 100755
--- a/tests/undefine
+++ b/tests/undefine
@@ -1,7 +1,7 @@
#!/bin/sh
# exercise virsh's "undefine" command
-# Copyright (C) 2008-2009 Red Hat, Inc.
+# Copyright (C) 2008-2010 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -34,7 +34,7 @@ $abs_top_builddir/tools/virsh -q -c test:///default undefine test >
out 2>&1
test $? = 1 || fail=1
cat <<\EOF > exp || fail=1
error: Failed to undefine domain test
-error: internal error Domain 'test' is still running
+error: internal error: Domain 'test' is still running
EOF
compare exp out || fail=1
--
1.7.0.1