[libvirt] [PATCH 0/3] Simplify error reporting int->string conversions
Currently the error reporting code has some horrible giant switch statements for doing integer to string conversions on error domains and error numbers. Replace this with the more traditional approach of using VIR_ENUM helpers
From: "Daniel P. Berrange" <berrange@redhat.com> The apibuild.py parser needs to be able to parse & ignore any VIR_ENUM_IMPL/VIR_ENUM_DECL macros in the source. Add some special case code to deal with this rather than trying to figure out a generic syntax for parsing macros. * apibuild.py: Special case VIR_ENUM_IMPL & VIR_ENUM_DECL --- docs/apibuild.py | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/docs/apibuild.py b/docs/apibuild.py index 1ac0281..05c2c8b 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -1355,6 +1355,95 @@ class CParser: token = self.token() return token + def parseVirEnumDecl(self, token): + if token[0] != "name": + self.error("parsing VIR_ENUM_DECL: expecting name", token) + + token = self.token() + + if token[0] != "sep": + self.error("parsing VIR_ENUM_DECL: expecting ')'", token) + + if token[1] != ')': + self.error("parsing VIR_ENUM_DECL: expecting ')'", token) + + token = self.token() + if token[0] == "sep" and token[1] == ';': + token = self.token() + + return token + + def parseVirEnumImpl(self, token): + # First the type name + if token[0] != "name": + self.error("parsing VIR_ENUM_IMPL: expecting name", token) + + token = self.token() + + if token[0] != "sep": + self.error("parsing VIR_ENUM_IMPL: expecting ','", token) + + if token[1] != ',': + self.error("parsing VIR_ENUM_IMPL: expecting ','", token) + token = self.token() + + # Now the sentinel name + if token[0] != "name": + self.error("parsing VIR_ENUM_IMPL: expecting name", token) + + token = self.token() + + if token[0] != "sep": + self.error("parsing VIR_ENUM_IMPL: expecting ','", token) + + if token[1] != ',': + self.error("parsing VIR_ENUM_IMPL: expecting ','", token) + + token = self.token() + + # Now a list of strings (optional comments) + while token is not None: + isGettext = False + # First a string, optionally with N_(...) + if token[0] == 'name': + if token[1] != 'N_': + self.error("parsing VIR_ENUM_IMPL: expecting 'N_'", token) + token = self.token() + if token[0] != "sep" or token[1] != '(': + self.error("parsing VIR_ENUM_IMPL: expecting '('", token) + token = self.token() + isGettext = True + + if token[0] != "string": + self.error("parsing VIR_ENUM_IMPL: expecting a string", token) + token = self.token() + elif token[0] == "string": + token = self.token() + else: + self.error("parsing VIR_ENUM_IMPL: expecting a string", token) + + # Then a separator + if token[0] == "sep": + if isGettext and token[1] == ')': + token = self.token() + + if token[1] == ',': + token = self.token() + + if token[1] == ')': + token = self.token() + break + + # Then an optional comment + if token[0] == "comment": + token = self.token() + + + if token[0] == "sep" and token[1] == ';': + token = self.token() + + return token + # # Parse a C definition block, used for structs or unions it parse till # the balancing } @@ -1502,6 +1591,29 @@ class CParser: not self.is_header, "enum", (enum[1], enum[2], enum_type)) return token + elif token[0] == "name" and token[1] == "VIR_ENUM_DECL": + token = self.token() + if token != None and token[0] == "sep" and token[1] == "(": + token = self.token() + token = self.parseVirEnumDecl(token) + else: + self.error("parsing VIR_ENUM_DECL: expecting '('", token) + if token != None: + self.lexer.push(token) + token = ("name", "virenumdecl") + return token + + elif token[0] == "name" and token[1] == "VIR_ENUM_IMPL": + token = self.token() + if token != None and token[0] == "sep" and token[1] == "(": + token = self.token() + token = self.parseVirEnumImpl(token) + else: + self.error("parsing VIR_ENUM_IMPL: expecting '('", token) + if token != None: + self.lexer.push(token) + token = ("name", "virenumimpl") + return token elif token[0] == "name": if self.type == "": -- 1.7.10.1
On 05/15/2012 05:30 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The apibuild.py parser needs to be able to parse & ignore any VIR_ENUM_IMPL/VIR_ENUM_DECL macros in the source. Add some special case code to deal with this rather than trying to figure out a generic syntax for parsing macros.
* apibuild.py: Special case VIR_ENUM_IMPL & VIR_ENUM_DECL --- docs/apibuild.py | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+)
I'm no python guru, but a compile smoke-test of this patch passed. weak ACK -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Add a VIR_ERR_DOMAIN_LAST sentinel for virErrorDomain and replace the virErrorDomainName function by a VIR_ENUM_IMPL In the process the naming of error domains is santized * src/util/virterror.c: Use VIR_ENUM_IMPL for converting error domains to strings * include/libvirt/virterror.h: Add VIR_ERR_DOMAIN_LAST --- include/libvirt/virterror.h | 13 +++ src/util/virterror.c | 219 +++++++++++++------------------------------ 2 files changed, 78 insertions(+), 154 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 7283207..0e0bc9c 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -43,51 +43,64 @@ typedef enum { VIR_FROM_XEND = 2, /* Error at connection with xend daemon */ VIR_FROM_XENSTORE = 3, /* Error at connection with xen store */ VIR_FROM_SEXPR = 4, /* Error in the S-Expression code */ + VIR_FROM_XML = 5, /* Error in the XML code */ VIR_FROM_DOM = 6, /* Error when operating on a domain */ VIR_FROM_RPC = 7, /* Error in the XML-RPC code */ VIR_FROM_PROXY = 8, /* Error in the proxy code; unused since 0.8.6 */ VIR_FROM_CONF = 9, /* Error in the configuration file handling */ + VIR_FROM_QEMU = 10, /* Error at the QEMU daemon */ VIR_FROM_NET = 11, /* Error when operating on a network */ VIR_FROM_TEST = 12, /* Error from test driver */ VIR_FROM_REMOTE = 13, /* Error from remote driver */ VIR_FROM_OPENVZ = 14, /* Error from OpenVZ driver */ + VIR_FROM_XENXM = 15, /* Error at Xen XM layer */ VIR_FROM_STATS_LINUX = 16, /* Error in the Linux Stats code */ VIR_FROM_LXC = 17, /* Error from Linux Container driver */ VIR_FROM_STORAGE = 18, /* Error from storage driver */ VIR_FROM_NETWORK = 19, /* Error from network config */ + VIR_FROM_DOMAIN = 20, /* Error from domain config */ VIR_FROM_UML = 21, /* Error at the UML driver */ VIR_FROM_NODEDEV = 22, /* Error from node device monitor */ VIR_FROM_XEN_INOTIFY = 23, /* Error from xen inotify layer */ VIR_FROM_SECURITY = 24, /* Error from security framework */ + VIR_FROM_VBOX = 25, /* Error from VirtualBox driver */ VIR_FROM_INTERFACE = 26, /* Error when operating on an interface */ VIR_FROM_ONE = 27, /* The OpenNebula driver no longer exists. Retained for ABI/API compat only */ VIR_FROM_ESX = 28, /* Error from ESX driver */ VIR_FROM_PHYP = 29, /* Error from IBM power hypervisor */ + VIR_FROM_SECRET = 30, /* Error from secret storage */ VIR_FROM_CPU = 31, /* Error from CPU driver */ VIR_FROM_XENAPI = 32, /* Error from XenAPI */ VIR_FROM_NWFILTER = 33, /* Error from network filter driver */ VIR_FROM_HOOK = 34, /* Error from Synchronous hooks */ + VIR_FROM_DOMAIN_SNAPSHOT = 35,/* Error from domain snapshot */ VIR_FROM_AUDIT = 36, /* Error from auditing subsystem */ VIR_FROM_SYSINFO = 37, /* Error from sysinfo/SMBIOS */ VIR_FROM_STREAMS = 38, /* Error from I/O streams */ VIR_FROM_VMWARE = 39, /* Error from VMware driver */ + VIR_FROM_EVENT = 40, /* Error from event loop impl */ VIR_FROM_LIBXL = 41, /* Error from libxenlight driver */ VIR_FROM_LOCKING = 42, /* Error from lock manager */ VIR_FROM_HYPERV = 43, /* Error from Hyper-V driver */ VIR_FROM_CAPABILITIES = 44, /* Error from capabilities */ + VIR_FROM_URI = 45, /* Error from URI handling */ VIR_FROM_AUTH = 46, /* Error from auth handling */ VIR_FROM_DBUS = 47, /* Error from DBus */ + +# ifdef VIR_ENUM_SENTINELS + VIR_ERR_DOMAIN_LAST +# endif } virErrorDomain; diff --git a/src/util/virterror.c b/src/util/virterror.c index b1a5d2b..fa57833 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -40,156 +40,67 @@ static virLogPriority virErrorLevelPriority(virErrorLevel level) { return VIR_LOG_ERROR; } -static const char *virErrorDomainName(virErrorDomain domain) { - const char *dom = "unknown"; - switch (domain) { - case VIR_FROM_NONE: - dom = ""; - break; - case VIR_FROM_XEN: - dom = "Xen "; - break; - case VIR_FROM_XENAPI: - dom = "XenAPI "; - break; - case VIR_FROM_LIBXL: - dom = "xenlight "; - break; - case VIR_FROM_XML: - dom = "XML "; - break; - case VIR_FROM_XEND: - dom = "Xen Daemon "; - break; - case VIR_FROM_XENSTORE: - dom = "Xen Store "; - break; - case VIR_FROM_XEN_INOTIFY: - dom = "Xen Inotify "; - break; - case VIR_FROM_DOM: - dom = "Domain "; - break; - case VIR_FROM_RPC: - dom = "RPC "; - break; - case VIR_FROM_QEMU: - dom = "QEMU "; - break; - case VIR_FROM_NET: - dom = "Network "; - break; - case VIR_FROM_TEST: - dom = "Test "; - break; - case VIR_FROM_REMOTE: - dom = "Remote "; - break; - case VIR_FROM_SEXPR: - dom = "S-Expr "; - break; - case VIR_FROM_PROXY: - dom = "PROXY "; - break; - case VIR_FROM_CONF: - dom = "Config "; - break; - case VIR_FROM_PHYP: - dom = "IBM power hypervisor "; - break; - case VIR_FROM_OPENVZ: - dom = "OpenVZ "; - break; - case VIR_FROM_VMWARE: - dom = "VMware "; - break; - case VIR_FROM_XENXM: - dom = "Xen XM "; - break; - case VIR_FROM_STATS_LINUX: - dom = "Linux Stats "; - break; - case VIR_FROM_LXC: - dom = "Linux Container "; - break; - case VIR_FROM_STORAGE: - dom = "Storage "; - break; - case VIR_FROM_NETWORK: - dom = "Network Config "; - break; - case VIR_FROM_DOMAIN: - dom = "Domain Config "; - break; - case VIR_FROM_NODEDEV: - dom = "Node Device "; - break; - case VIR_FROM_UML: - dom = "UML "; - break; - case VIR_FROM_SECURITY: - dom = "Security Labeling "; - break; - case VIR_FROM_VBOX: - dom = "VBOX "; - break; - case VIR_FROM_INTERFACE: - dom = "Interface "; - break; - case VIR_FROM_ONE: - dom = "ONE "; - break; - case VIR_FROM_ESX: - dom = "ESX "; - break; - case VIR_FROM_SECRET: - dom = "Secret Storage "; - break; - case VIR_FROM_CPU: - dom = "CPU "; - break; - case VIR_FROM_NWFILTER: - dom = "Network Filter "; - break; - case VIR_FROM_HOOK: - dom = "Sync Hook "; - break; - case VIR_FROM_DOMAIN_SNAPSHOT: - dom = "Domain Snapshot "; - break; - case VIR_FROM_AUDIT: - dom = "Audit "; - break; - case VIR_FROM_SYSINFO: - dom = "Sysinfo "; - break; - case VIR_FROM_STREAMS: - dom = "Streams "; - break; - case VIR_FROM_EVENT: - dom = "Events "; - break; - case VIR_FROM_LOCKING: - dom = "Locking "; - break; - case VIR_FROM_HYPERV: - dom = "Hyper-V "; - break; - case VIR_FROM_CAPABILITIES: - dom = "Capabilities "; - break; - case VIR_FROM_URI: - dom = "URI "; - break; - case VIR_FROM_AUTH: - dom = "Auth "; - break; - case VIR_FROM_DBUS: - dom = "DBus "; - break; - } - return dom; -} + +VIR_ENUM_DECL(virErrorDomain) +VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, + "", /* 0 */ + "Xen Driver", + "Xen Daemon", + "Xen Store", + "S-Expression", + + "XML Util", /* 5 */ + "Domain", + "XML-RPC", + "Proxy Daemon", + "Config File", + + "QEMU Driver", /* 10 */ + "Network", + "Test Driver", + "Remote Driver", + "OpenVZ Driver", + + "Xen XM Driver", /* 15 */ + "Linux Statistics", + "LXC Driver", + "Storage Driver", + "Network Driver", + + "Domain Config", /* 20 */ + "User Mode Linux Driver", + "Node Device Driver", + "Xen Inotify Driver", + "Security Driver", + + "VirtualBox Driver", /* 25 */ + "Network Interface Driver", + "Open Nebula Driver", + "ESX Driver", + "Power Hypervisor Driver", + + "Secrets Driver", /* 30 */ + "CPU Driver", + "XenAPI Driver", + "Network Filter Driver", + "Lifecycle Hook", + + "Domain Snapshot", /* 35 */ + "Audit Utils", + "Sysinfo Utils", + "I/O Stream Utils", + "VMWare Driver", + + "Event Loop", /* 40 */ + "Xen Light Driver", + "Lock Driver", + "Hyper-V Driver", + "Capabilities Utils", + + "URI Utils", /* 45 */ + "Authentication Utils", + "DBus Utils" + ) /* @@ -585,7 +496,7 @@ virDefaultErrorFunc(virErrorPtr err) lvl = _("error"); break; } - dom = virErrorDomainName(err->domain); + dom = virErrorDomainTypeToString(err->domain); if ((err->dom != NULL) && (err->code != VIR_ERR_INVALID_DOMAIN)) { domain = err->dom->name; } else if ((err->net != NULL) && (err->code != VIR_ERR_INVALID_NETWORK)) { @@ -594,13 +505,13 @@ virDefaultErrorFunc(virErrorPtr err) len = strlen(err->message); if ((err->domain == VIR_FROM_XML) && (err->code == VIR_ERR_XML_DETAIL) && (err->int1 != 0)) - fprintf(stderr, "libvir: %s%s %s%s: line %d: %s", + fprintf(stderr, "libvir: %s %s %s%s: line %d: %s", dom, lvl, domain, network, err->int1, err->message); else if ((len == 0) || (err->message[len - 1] != '\n')) - fprintf(stderr, "libvir: %s%s %s%s: %s\n", + fprintf(stderr, "libvir: %s %s %s%s: %s\n", dom, lvl, domain, network, err->message); else - fprintf(stderr, "libvir: %s%s %s%s: %s", + fprintf(stderr, "libvir: %s %s %s%s: %s", dom, lvl, domain, network, err->message); } -- 1.7.10.1
On 05/15/2012 05:30 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Add a VIR_ERR_DOMAIN_LAST sentinel for virErrorDomain and replace the virErrorDomainName function by a VIR_ENUM_IMPL
In the process the naming of error domains is santized
s/santized/sanitized/
* src/util/virterror.c: Use VIR_ENUM_IMPL for converting error domains to strings * include/libvirt/virterror.h: Add VIR_ERR_DOMAIN_LAST --- include/libvirt/virterror.h | 13 +++ src/util/virterror.c | 219 +++++++++++++------------------------------ 2 files changed, 78 insertions(+), 154 deletions(-)
Oh my - I dug through my old branches, and saw that I tried (and failed) to do this type of cleanup 18 months ago. Your version is cleaner than my attempt; I like it.
/* @@ -585,7 +496,7 @@ virDefaultErrorFunc(virErrorPtr err) lvl = _("error"); break; } - dom = virErrorDomainName(err->domain); + dom = virErrorDomainTypeToString(err->domain);
You are missing a check for dom==NULL (possible if a newer server sends an error domain we are not familiar with); you should have a catch-all case that converts NULL to "Unknown". ACK with that tweak. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Add a VIR_ERR_NUMBER_LAST sentinel for virErrorNumber and replace the virErrorStr function by a VIR_ENUM_IMPL. Once this is done all callers of virRaiseErrorFull are simply passing in a constant string message with "%s" as the format arg. Thus virRaiseErrorFull can have its var-args removed. * include/libvirt/virterror.h: Add VIR_ERR_NUMBER_LAST sentinel * src/rpc/virnetclientprogram.c, src/rpc/virnetclientstream.c: Remove format string passed to virRaiseErrorFull * src/util/virterror.c: Remove var-args from virRaiseErrorFull and replace virErrorStr with an VIR_ENUM * src/util/virterror_internal.h: Remove var-args from virRaiseErrorFull * tests/cpuset, tests/undefine, tests/virsh-optparse: Adapt for changes in error message --- include/libvirt/virterror.h | 37 +++ src/rpc/virnetclientprogram.c | 4 +- src/rpc/virnetclientstream.c | 2 +- src/util/virterror.c | 702 +++++++++-------------------------------- src/util/virterror_internal.h | 3 +- tests/cpuset | 2 +- tests/undefine | 2 +- tests/virsh-optparse | 2 +- 8 files changed, 193 insertions(+), 561 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 0e0bc9c..6ae1f6d 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -172,27 +172,37 @@ typedef enum { VIR_ERR_NO_MEMORY = 2, /* memory allocation failure */ VIR_ERR_NO_SUPPORT = 3, /* no support for this function */ VIR_ERR_UNKNOWN_HOST = 4, /* could not resolve hostname */ + + VIR_ERR_NO_CONNECT = 5, /* can't connect to hypervisor */ VIR_ERR_INVALID_CONN = 6, /* invalid connection object */ VIR_ERR_INVALID_DOMAIN = 7, /* invalid domain object */ VIR_ERR_INVALID_ARG = 8, /* invalid function argument */ VIR_ERR_OPERATION_FAILED = 9, /* a command to hypervisor failed */ + + VIR_ERR_GET_FAILED = 10, /* a HTTP GET command to failed */ VIR_ERR_POST_FAILED = 11, /* a HTTP POST command to failed */ VIR_ERR_HTTP_ERROR = 12, /* unexpected HTTP error code */ VIR_ERR_SEXPR_SERIAL = 13, /* failure to serialize an S-Expr */ VIR_ERR_NO_XEN = 14, /* could not open Xen hypervisor control */ + + VIR_ERR_XEN_CALL = 15, /* failure doing an hypervisor call */ VIR_ERR_OS_TYPE = 16, /* unknown OS type */ VIR_ERR_NO_KERNEL = 17, /* missing kernel information */ VIR_ERR_NO_ROOT = 18, /* missing root device information */ VIR_ERR_NO_SOURCE = 19, /* missing source device information */ + + VIR_ERR_NO_TARGET = 20, /* missing target device information */ VIR_ERR_NO_NAME = 21, /* missing domain name information */ VIR_ERR_NO_OS = 22, /* missing domain OS information */ VIR_ERR_NO_DEVICE = 23, /* missing domain devices information */ VIR_ERR_NO_XENSTORE = 24, /* could not open Xen Store control */ + + VIR_ERR_DRIVER_FULL = 25, /* too many drivers registered */ VIR_ERR_CALL_FAILED = 26, /* not supported by the drivers (DEPRECATED) */ @@ -201,33 +211,45 @@ typedef enum { VIR_ERR_DOM_EXIST = 28, /* the domain already exist */ VIR_ERR_OPERATION_DENIED = 29, /* operation forbidden on read-only connections */ + + VIR_ERR_OPEN_FAILED = 30, /* failed to open a conf file */ VIR_ERR_READ_FAILED = 31, /* failed to read a conf file */ VIR_ERR_PARSE_FAILED = 32, /* failed to parse a conf file */ VIR_ERR_CONF_SYNTAX = 33, /* failed to parse the syntax of a conf file */ VIR_ERR_WRITE_FAILED = 34, /* failed to write a conf file */ + + VIR_ERR_XML_DETAIL = 35, /* detail of an XML error */ VIR_ERR_INVALID_NETWORK = 36, /* invalid network object */ VIR_ERR_NETWORK_EXIST = 37, /* the network already exist */ VIR_ERR_SYSTEM_ERROR = 38, /* general system call failure */ VIR_ERR_RPC = 39, /* some sort of RPC error */ + + VIR_ERR_GNUTLS_ERROR = 40, /* error from a GNUTLS call */ VIR_WAR_NO_NETWORK = 41, /* failed to start network */ VIR_ERR_NO_DOMAIN = 42, /* domain not found or unexpectedly disappeared */ VIR_ERR_NO_NETWORK = 43, /* network not found */ VIR_ERR_INVALID_MAC = 44, /* invalid MAC address */ + + VIR_ERR_AUTH_FAILED = 45, /* authentication failed */ VIR_ERR_INVALID_STORAGE_POOL = 46, /* invalid storage pool object */ VIR_ERR_INVALID_STORAGE_VOL = 47, /* invalid storage vol object */ VIR_WAR_NO_STORAGE = 48, /* failed to start storage */ VIR_ERR_NO_STORAGE_POOL = 49, /* storage pool not found */ + + VIR_ERR_NO_STORAGE_VOL = 50, /* storage volume not found */ VIR_WAR_NO_NODE = 51, /* failed to start node driver */ VIR_ERR_INVALID_NODE_DEVICE = 52, /* invalid node device object */ VIR_ERR_NO_NODE_DEVICE = 53, /* node device not found */ VIR_ERR_NO_SECURITY_MODEL = 54, /* security model not found */ + + VIR_ERR_OPERATION_INVALID = 55, /* operation is not applicable at this time */ VIR_WAR_NO_INTERFACE = 56, /* failed to start interface driver */ @@ -235,11 +257,15 @@ typedef enum { VIR_ERR_INVALID_INTERFACE = 58, /* invalid interface object */ VIR_ERR_MULTIPLE_INTERFACES = 59, /* more than one matching interface found */ + + VIR_WAR_NO_NWFILTER = 60, /* failed to start nwfilter driver */ VIR_ERR_INVALID_NWFILTER = 61, /* invalid nwfilter object */ VIR_ERR_NO_NWFILTER = 62, /* nw filter pool not found */ VIR_ERR_BUILD_FIREWALL = 63, /* nw filter pool not found */ VIR_WAR_NO_SECRET = 64, /* failed to start secret storage */ + + VIR_ERR_INVALID_SECRET = 65, /* invalid secret */ VIR_ERR_NO_SECRET = 66, /* secret not found */ VIR_ERR_CONFIG_UNSUPPORTED = 67, /* unsupported configuration @@ -247,12 +273,16 @@ typedef enum { VIR_ERR_OPERATION_TIMEOUT = 68, /* timeout occurred during operation */ VIR_ERR_MIGRATE_PERSIST_FAILED = 69,/* a migration worked, but making the VM persist on the dest host failed */ + + VIR_ERR_HOOK_SCRIPT_FAILED = 70, /* a synchronous hook script failed */ VIR_ERR_INVALID_DOMAIN_SNAPSHOT = 71,/* invalid domain snapshot */ VIR_ERR_NO_DOMAIN_SNAPSHOT = 72, /* domain snapshot not found */ VIR_ERR_INVALID_STREAM = 73, /* stream pointer not valid */ VIR_ERR_ARGUMENT_UNSUPPORTED = 74, /* valid API use but unsupported by the given driver */ + + VIR_ERR_STORAGE_PROBE_FAILED = 75, /* storage pool probe failed */ VIR_ERR_STORAGE_POOL_BUILT = 76, /* storage pool already built */ VIR_ERR_SNAPSHOT_REVERT_RISKY = 77, /* force was not requested for a @@ -260,10 +290,17 @@ typedef enum { VIR_ERR_OPERATION_ABORTED = 78, /* operation on a domain was canceled/aborted by user */ VIR_ERR_AUTH_CANCELLED = 79, /* authentication cancelled */ + + VIR_ERR_NO_DOMAIN_METADATA = 80, /* The metadata is not present */ VIR_ERR_MIGRATE_UNSAFE = 81, /* Migration is not safe */ VIR_ERR_OVERFLOW = 82, /* integer overflow */ VIR_ERR_BLOCK_COPY_ACTIVE = 83, /* action prevented by block copy job */ + + +# ifdef VIR_ENUM_SENTINELS + VIR_ERR_NUMBER_LAST +# endif } virErrorNumber; /** diff --git a/src/rpc/virnetclientprogram.c b/src/rpc/virnetclientprogram.c index e1e8846..383c0f5 100644 --- a/src/rpc/virnetclientprogram.c +++ b/src/rpc/virnetclientprogram.c @@ -169,7 +169,7 @@ virNetClientProgramDispatchError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, err.str3 ? *err.str3 : NULL, err.int1, err.int2, - "%s", *err.message); + *err.message); } else { virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, err.domain, @@ -180,7 +180,7 @@ virNetClientProgramDispatchError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, err.str3 ? *err.str3 : NULL, err.int1, err.int2, - "%s", err.message ? *err.message : _("Unknown error")); + err.message ? *err.message : NULL); } ret = 0; diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c index be06c66..84ed5d7 100644 --- a/src/rpc/virnetclientstream.c +++ b/src/rpc/virnetclientstream.c @@ -215,7 +215,7 @@ bool virNetClientStreamRaiseError(virNetClientStreamPtr st) st->err.str3, st->err.int1, st->err.int2, - "%s", st->err.message ? st->err.message : _("Unknown error")); + st->err.message); virMutexUnlock(&st->lock); return true; } diff --git a/src/util/virterror.c b/src/util/virterror.c index fa57833..1449863 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -21,6 +21,7 @@ #include "memory.h" #include "threads.h" #include "util.h" +#include "ignore-value.h" virThreadLocal virLastErr; @@ -103,6 +104,110 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, ) +VIR_ENUM_DECL(virErrorNumber) +VIR_ENUM_IMPL(virErrorNumber, VIR_ERR_NUMBER_LAST, + N_(""), /* 0 */ + N_("internal error"), + N_("out of memory"), + N_("this function is not supported by the current driver"), + N_("unknown hostname"), + + N_("no connection driver available"), /* 5 */ + N_("invalid connection pointer"), + N_("invalid domain pointer"), + N_("invalid argument"), + N_("operation failed"), + + N_("HTTP GET request failed"), /* 10 */ + N_("HTTP POST request failed"), + N_("unknown HTTP error code"), + N_("failed to serialize S-expression"), + N_("unable to use Xen hypervisor"), + + N_("failed Xen hypercall"), /* 15 */ + N_("unknown OS type"), + N_("missing kernel path"), + N_("missing root device name"), + N_("missing source information for device"), + + N_("missing target information for device"), /* 20 */ + N_("missing name information"), + N_("missing operating system information"), + N_("missing device list"), + N_("unable to use XenStore daemon"), + + N_("too many drivers registered"), /* 25 */ + N_("library call failed, possibly not supported"), + N_("XML description is invalid or not well formed"), + N_("domain exists already"), + N_("operation forbidden for read only access"), + + N_("failed to open configuration file"), /* 30 */ + N_("failed to read configuration file"), + N_("failed to parse configuration file"), + N_("configuration file syntax error"), + N_("failed to write configuration file"), + + N_("XML parser error"), /* 35 */ + N_("invalid network pointer"), + N_("network exists already"), + N_("system call error"), + N_("RPC error"), + + N_("GNUTLS call error"), /* 40 */ + N_("failed to find a network driver"), + N_("domain not found"), + N_("network not found"), + N_("invalid MAC address"), + + N_("authentication failed"), /* 45 */ + N_("invalid storage pool pointer"), + N_("invalid storage volume pointer"), + N_("failed to find a storage driver"), + N_("storage pool not found"), + + N_("storage volume not found"), /* 50 */ + N_("failed to find a node device driver"), + N_("invalid node device pointer"), + N_("node device not found"), + N_("security model not found"), + + N_("requested operation is not valid"), /* 55 */ + N_("failed to find a network interface driver"), + N_("interface not found"), + N_("invalid interface pointer"), + N_("multiple matching interfaces found"), + + N_("failed to find a network filter driver"), /* 60 */ + N_("invalid network filter"), + N_("network filter not found"), + N_("error while building firewall"), + N_("failed to find a secret driver"), + + N_("invalid secret pointer"), /* 65 */ + N_("secret not found"), + N_("unsupported configuration"), + N_("timed out during operation"), + N_("failed to make domain persistent after migration"), + + N_("hook script execution failed"), /* 70 */ + N_("invalid snapshot pointer"), + N_("domain snapshot not found"), + N_("invalid stream pointer"), + N_("argument unsupported"), + + N_("storage pool probe failed"), /* 75 */ + N_("storage pool already built"), + N_("revert requires force"), + N_("operation aborted"), + N_("authentication cancelled"), + + N_("metadata not found"), /* 80 */ + N_("unsafe migration"), + N_("numerical overflow"), + N_("block copy still active"), + ) + /* * Internal helper that is called when a thread exits, to * release the error object stored in the thread local @@ -576,8 +681,7 @@ virDispatchError(virConnectPtr conn) * @str3: extra string info * @int1: extra int info * @int2: extra int info - * @fmt: the message to display/transmit - * @...: extra parameters for the message display + * @msg: the pre-formatted message to display/transmit * * Internal routine called when an error is detected. It will raise it * immediately if a callback is found and store it for later handling. @@ -594,11 +698,10 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, const char *str3, int int1, int int2, - const char *fmt, ...) + const char *msg) { int save_errno = errno; virErrorPtr to; - char *str; int priority; /* @@ -620,18 +723,6 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, } /* - * formats the message; drop message on OOM situations - */ - if (fmt == NULL) { - str = strdup(_("No error message provided")); - } else { - va_list ap; - va_start(ap, fmt); - virVasprintf(&str, fmt, ap); - va_end(ap); - } - - /* * Save the information about the error */ /* @@ -640,7 +731,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, */ to->domain = domain; to->code = code; - to->message = str; + to->message = strdup(msg); to->level = level; if (str1 != NULL) to->str1 = strdup(str1); @@ -661,518 +752,11 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, virLogMessage(filename, priority, funcname, linenr, virErrorLogPriorityFilter ? 0 : 1, - "%s", str); + "%s", msg); errno = save_errno; } -/** - * virErrorMsg: - * @error: the virErrorNumber - * @info: usually the first parameter string - * - * Internal routine to get the message associated to an error raised - * from the library - * - * Returns the constant string associated to @error - */ -static const char * -virErrorMsg(virErrorNumber error, const char *info) -{ - const char *errmsg = NULL; - - switch (error) { - case VIR_ERR_OK: - return NULL; - case VIR_ERR_INTERNAL_ERROR: - if (info != NULL) - errmsg = _("internal error %s"); - else - 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 connection driver"); - else - errmsg = _("this function is not supported by the connection driver: %s"); - break; - case VIR_ERR_NO_CONNECT: - if (info == NULL) - errmsg = _("no connection driver available"); - else - errmsg = _("no connection driver available for %s"); - break; - case VIR_ERR_INVALID_CONN: - if (info == NULL) - errmsg = _("invalid connection pointer in"); - else - errmsg = _("invalid connection pointer in %s"); - break; - case VIR_ERR_INVALID_DOMAIN: - if (info == NULL) - errmsg = _("invalid domain pointer in"); - else - errmsg = _("invalid domain pointer in %s"); - break; - case VIR_ERR_INVALID_ARG: - if (info == NULL) - errmsg = _("invalid argument"); - else - errmsg = _("invalid argument: %s"); - break; - case VIR_ERR_OPERATION_FAILED: - if (info != NULL) - errmsg = _("operation failed: %s"); - else - errmsg = _("operation failed"); - break; - case VIR_ERR_GET_FAILED: - if (info != NULL) - errmsg = _("GET operation failed: %s"); - else - errmsg = _("GET operation failed"); - break; - case VIR_ERR_POST_FAILED: - if (info != NULL) - errmsg = _("POST operation failed: %s"); - else - errmsg = _("POST operation failed"); - break; - case VIR_ERR_HTTP_ERROR: - errmsg = _("got unknown HTTP error code %d"); - break; - case VIR_ERR_UNKNOWN_HOST: - if (info != NULL) - errmsg = _("unknown host %s"); - else - 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"); - 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"); - 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"); - break; - case VIR_ERR_XEN_CALL: - errmsg = _("failed Xen syscall %s"); - break; - case VIR_ERR_OS_TYPE: - if (info == NULL) - errmsg = _("unknown OS type"); - else - errmsg = _("unknown OS type %s"); - 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"); - break; - case VIR_ERR_NO_SOURCE: - if (info == NULL) - errmsg = _("missing source information for device"); - else - errmsg = _("missing source information for device %s"); - break; - case VIR_ERR_NO_TARGET: - if (info == NULL) - errmsg = _("missing target information for device"); - else - errmsg = _("missing target information for device %s"); - break; - case VIR_ERR_NO_NAME: - if (info == NULL) - errmsg = _("missing name information"); - else - errmsg = _("missing name information in %s"); - break; - case VIR_ERR_NO_OS: - if (info == NULL) - errmsg = _("missing operating system information"); - else - errmsg = _("missing operating system information for %s"); - break; - case VIR_ERR_NO_DEVICE: - if (info == NULL) - errmsg = _("missing devices information"); - else - errmsg = _("missing devices information for %s"); - 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"); - break; - case VIR_ERR_XML_ERROR: - if (info == NULL) - errmsg = _("XML description is invalid or not well formed"); - else - errmsg = _("XML error: %s"); - break; - case VIR_ERR_DOM_EXIST: - if (info == NULL) - errmsg = _("this domain exists already"); - else - errmsg = _("domain %s exists already"); - break; - case VIR_ERR_OPERATION_DENIED: - if (info == NULL) - errmsg = _("operation forbidden for read only access"); - else - errmsg = _("operation %s forbidden for read only access"); - break; - case VIR_ERR_OPEN_FAILED: - if (info == NULL) - errmsg = _("failed to open configuration file for reading"); - else - errmsg = _("failed to open %s for reading"); - break; - case VIR_ERR_READ_FAILED: - if (info == NULL) - errmsg = _("failed to read configuration file"); - else - errmsg = _("failed to read configuration file %s"); - break; - case VIR_ERR_PARSE_FAILED: - if (info == NULL) - errmsg = _("failed to parse configuration file"); - else - errmsg = _("failed to parse configuration file %s"); - break; - case VIR_ERR_CONF_SYNTAX: - if (info == NULL) - errmsg = _("configuration file syntax error"); - else - errmsg = _("configuration file syntax error: %s"); - break; - case VIR_ERR_WRITE_FAILED: - if (info == NULL) - errmsg = _("failed to write configuration file"); - else - errmsg = _("failed to write configuration file: %s"); - break; - case VIR_ERR_XML_DETAIL: - if (info == NULL) - errmsg = _("parser error"); - else - errmsg = "%s"; - break; - case VIR_ERR_INVALID_NETWORK: - if (info == NULL) - errmsg = _("invalid network pointer in"); - else - errmsg = _("invalid network pointer in %s"); - break; - case VIR_ERR_NETWORK_EXIST: - if (info == NULL) - errmsg = _("this network exists already"); - else - errmsg = _("network %s exists already"); - break; - case VIR_ERR_SYSTEM_ERROR: - if (info == NULL) - errmsg = _("system call error"); - else - errmsg = "%s"; - break; - case VIR_ERR_RPC: - if (info == NULL) - errmsg = _("RPC error"); - else - errmsg = "%s"; - break; - case VIR_ERR_GNUTLS_ERROR: - if (info == NULL) - errmsg = _("GNUTLS call error"); - else - errmsg = "%s"; - break; - case VIR_WAR_NO_NETWORK: - if (info == NULL) - errmsg = _("Failed to find the network"); - else - errmsg = _("Failed to find the network: %s"); - break; - case VIR_ERR_NO_DOMAIN: - if (info == NULL) - errmsg = _("Domain not found"); - else - errmsg = _("Domain not found: %s"); - break; - case VIR_ERR_NO_NETWORK: - if (info == NULL) - errmsg = _("Network not found"); - else - errmsg = _("Network not found: %s"); - break; - case VIR_ERR_INVALID_MAC: - if (info == NULL) - errmsg = _("invalid MAC address"); - else - errmsg = _("invalid MAC address: %s"); - break; - case VIR_ERR_AUTH_FAILED: - if (info == NULL) - errmsg = _("authentication failed"); - else - errmsg = _("authentication failed: %s"); - break; - case VIR_ERR_AUTH_CANCELLED: - if (info == NULL) - errmsg = _("authentication cancelled"); - else - errmsg = _("authentication cancelled: %s"); - break; - case VIR_ERR_NO_STORAGE_POOL: - if (info == NULL) - errmsg = _("Storage pool not found"); - else - errmsg = _("Storage pool not found: %s"); - break; - case VIR_ERR_NO_STORAGE_VOL: - if (info == NULL) - errmsg = _("Storage volume not found"); - else - errmsg = _("Storage volume not found: %s"); - break; - case VIR_ERR_STORAGE_PROBE_FAILED: - if (info == NULL) - errmsg = _("Storage pool probe failed"); - else - errmsg = _("Storage pool probe failed: %s"); - break; - case VIR_ERR_STORAGE_POOL_BUILT: - if (info == NULL) - errmsg = _("Storage pool already built"); - else - errmsg = _("Storage pool already built: %s"); - break; - case VIR_ERR_INVALID_STORAGE_POOL: - if (info == NULL) - errmsg = _("invalid storage pool pointer in"); - else - errmsg = _("invalid storage pool pointer in %s"); - break; - case VIR_ERR_INVALID_STORAGE_VOL: - if (info == NULL) - errmsg = _("invalid storage volume pointer in"); - else - errmsg = _("invalid storage volume pointer in %s"); - 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"); - 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"); - break; - case VIR_ERR_INVALID_NODE_DEVICE: - if (info == NULL) - errmsg = _("invalid node device pointer"); - else - errmsg = _("invalid node device pointer in %s"); - break; - case VIR_ERR_NO_NODE_DEVICE: - if (info == NULL) - errmsg = _("Node device not found"); - else - errmsg = _("Node device not found: %s"); - break; - case VIR_ERR_NO_SECURITY_MODEL: - if (info == NULL) - errmsg = _("Security model not found"); - else - errmsg = _("Security model not found: %s"); - break; - case VIR_ERR_OPERATION_INVALID: - if (info == NULL) - errmsg = _("Requested operation is not valid"); - else - errmsg = _("Requested operation is not valid: %s"); - break; - case VIR_WAR_NO_INTERFACE: - if (info == NULL) - errmsg = _("Failed to find the interface"); - else - errmsg = _("Failed to find the interface: %s"); - break; - case VIR_ERR_NO_INTERFACE: - if (info == NULL) - errmsg = _("Interface not found"); - else - errmsg = _("Interface not found: %s"); - break; - case VIR_ERR_INVALID_INTERFACE: - if (info == NULL) - errmsg = _("invalid interface pointer in"); - else - errmsg = _("invalid interface pointer in %s"); - break; - case VIR_ERR_MULTIPLE_INTERFACES: - if (info == NULL) - errmsg = _("multiple matching interfaces found"); - else - errmsg = _("multiple matching interfaces found: %s"); - 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"); - break; - case VIR_ERR_INVALID_SECRET: - if (info == NULL) - errmsg = _("Invalid secret"); - else - errmsg = _("Invalid secret: %s"); - break; - case VIR_ERR_NO_SECRET: - if (info == NULL) - errmsg = _("Secret not found"); - else - errmsg = _("Secret not found: %s"); - 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"); - break; - case VIR_ERR_INVALID_NWFILTER: - if (info == NULL) - errmsg = _("Invalid network filter"); - else - errmsg = _("Invalid network filter: %s"); - break; - case VIR_ERR_NO_NWFILTER: - if (info == NULL) - errmsg = _("Network filter not found"); - else - errmsg = _("Network filter not found: %s"); - break; - case VIR_ERR_BUILD_FIREWALL: - if (info == NULL) - errmsg = _("Error while building firewall"); - else - errmsg = _("Error while building firewall: %s"); - break; - case VIR_ERR_CONFIG_UNSUPPORTED: - if (info == NULL) - errmsg = _("unsupported configuration"); - else - errmsg = _("unsupported configuration: %s"); - break; - case VIR_ERR_OPERATION_TIMEOUT: - if (info == NULL) - errmsg = _("Timed out during operation"); - else - errmsg = _("Timed out during operation: %s"); - 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"); - break; - case VIR_ERR_HOOK_SCRIPT_FAILED: - if (info == NULL) - errmsg = _("Hook script execution failed"); - else - errmsg = _("Hook script execution failed: %s"); - break; - case VIR_ERR_INVALID_DOMAIN_SNAPSHOT: - if (info == NULL) - errmsg = _("Invalid snapshot"); - else - errmsg = _("Invalid snapshot: %s"); - break; - case VIR_ERR_NO_DOMAIN_SNAPSHOT: - if (info == NULL) - errmsg = _("Domain snapshot not found"); - else - errmsg = _("Domain snapshot not found: %s"); - break; - case VIR_ERR_INVALID_STREAM: - if (info == NULL) - errmsg = _("invalid stream pointer"); - else - errmsg = _("invalid stream pointer in %s"); - break; - case VIR_ERR_ARGUMENT_UNSUPPORTED: - if (info == NULL) - errmsg = _("argument unsupported"); - else - errmsg = _("argument unsupported: %s"); - break; - case VIR_ERR_SNAPSHOT_REVERT_RISKY: - if (info == NULL) - errmsg = _("revert requires force"); - else - errmsg = _("revert requires force: %s"); - break; - case VIR_ERR_OPERATION_ABORTED: - if (info == NULL) - errmsg = _("operation aborted"); - else - errmsg = _("operation aborted: %s"); - break; - case VIR_ERR_NO_DOMAIN_METADATA: - if (info == NULL) - errmsg = _("metadata not found"); - else - errmsg = _("metadata not found: %s"); - break; - case VIR_ERR_MIGRATE_UNSAFE: - if (!info) - errmsg = _("Unsafe migration"); - else - errmsg = _("Unsafe migration: %s"); - break; - case VIR_ERR_OVERFLOW: - if (!info) - errmsg = _("numerical overflow"); - else - errmsg = _("numerical overflow: %s"); - break; - case VIR_ERR_BLOCK_COPY_ACTIVE: - if (!info) - errmsg = _("block copy still active"); - else - errmsg = _("block copy still active: %s"); - break; - } - return errmsg; -} /** * virReportErrorHelper: @@ -1196,23 +780,41 @@ void virReportErrorHelper(int domcode, const char *fmt, ...) { int save_errno = errno; - va_list args; - char errorMessage[1024]; - const char *virerr; + char *msg = NULL; + char *fmtmsg = NULL; + const char *virerr = gettext(virErrorNumberTypeToString(errorcode)); + + /* Special case a couple of codes where we don't want to + * prepend some worthless generic message, to a detailed + * piece of info */ + if ((errorcode == VIR_ERR_GNUTLS_ERROR || + errorcode == VIR_ERR_RPC || + errorcode == VIR_ERR_SYSTEM_ERROR) + && fmt) + virerr = NULL; if (fmt) { + va_list args; + va_start(args, fmt); - vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); + ignore_value(virVasprintf(&fmtmsg, fmt, args)); va_end(args); - } else { - errorMessage[0] = '\0'; + + if (fmtmsg && virerr) { + ignore_value(virAsprintf(&msg, "%s: %s", virerr, fmtmsg)); + } else if (fmtmsg) { + msg = fmtmsg; + fmtmsg = NULL; + } } - virerr = virErrorMsg(errorcode, (errorMessage[0] ? errorMessage : NULL)); virRaiseErrorFull(filename, funcname, linenr, domcode, errorcode, VIR_ERR_ERROR, - virerr, errorMessage, NULL, - -1, -1, virerr, errorMessage); + virerr, fmtmsg, NULL, + -1, -1, + msg ? msg : virerr); + VIR_FREE(msg); + VIR_FREE(fmtmsg); errno = save_errno; } @@ -1259,35 +861,30 @@ void virReportSystemErrorFull(int domcode, { int save_errno = errno; char strerror_buf[1024]; - char msgDetailBuf[1024]; - + const char *virerr = gettext(virErrorNumberTypeToString(VIR_ERR_SYSTEM_ERROR)); const char *errnoDetail = virStrerror(theerrno, strerror_buf, sizeof(strerror_buf)); - const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); - const char *msgDetail = NULL; + char *msg = NULL; + char *fmtmsg; if (fmt) { va_list args; - int n; va_start(args, fmt); - n = vsnprintf(msgDetailBuf, sizeof(msgDetailBuf), fmt, args); + ignore_value(virVasprintf(&fmtmsg, fmt, args)); va_end(args); - size_t len = strlen(errnoDetail); - if (0 <= n && n + 2 + len < sizeof(msgDetailBuf)) { - char *p = msgDetailBuf + n; - stpcpy (stpcpy (p, ": "), errnoDetail); - msgDetail = msgDetailBuf; - } + if (fmtmsg) + ignore_value(virAsprintf(&msg, "%s: %s", fmtmsg, errnoDetail)); } - if (!msgDetail) - msgDetail = errnoDetail; - virRaiseErrorFull(filename, funcname, linenr, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, - msg, msgDetail, NULL, -1, -1, msg, msgDetail); + virerr, fmtmsg, errnoDetail, + -1, -1, + msg ? msg : errnoDetail); + VIR_FREE(fmtmsg); + VIR_FREE(msg); errno = save_errno; } @@ -1307,11 +904,10 @@ void virReportOOMErrorFull(int domcode, size_t linenr) { const char *virerr; - - virerr = virErrorMsg(VIR_ERR_NO_MEMORY, NULL); + virerr = gettext(virErrorNumberTypeToString(VIR_ERR_NO_MEMORY)); virRaiseErrorFull(filename, funcname, linenr, domcode, VIR_ERR_NO_MEMORY, VIR_ERR_ERROR, - virerr, NULL, NULL, -1, -1, virerr, NULL); + virerr, NULL, NULL, -1, -1, virerr); } /** diff --git a/src/util/virterror_internal.h b/src/util/virterror_internal.h index b8cb279..ffa2d02 100644 --- a/src/util/virterror_internal.h +++ b/src/util/virterror_internal.h @@ -44,8 +44,7 @@ void virRaiseErrorFull(const char *filename, const char *str3, int int1, int int2, - const char *fmt, ...) - ATTRIBUTE_FMT_PRINTF(12, 13); + const char *msg); void virReportErrorHelper(int domcode, int errcode, const char *filename, diff --git a/tests/cpuset b/tests/cpuset index d638ad6..26f97f0 100755 --- a/tests/cpuset +++ b/tests/cpuset @@ -41,7 +41,7 @@ sed "s/vcpu placement='static'>/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 22d6c14..59771b6 100755 --- a/tests/undefine +++ b/tests/undefine @@ -64,7 +64,7 @@ cat <<\EOF > expout || fail=1 Domain test is being shutdown Domain test has been undefined error: failed to get domain 'test' -error: Domain not found +error: domain not found EOF compare expout out || fail=1 diff --git a/tests/virsh-optparse b/tests/virsh-optparse index fc2279f..ce8dd87 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -129,7 +129,7 @@ test -s err && fail=1 # Test a required argv cat <<\EOF > exp-err || framework_failure -error: this function is not supported by the connection driver: virDomainQemuMonitorCommand +error: this function is not supported by the current driver: virDomainQemuMonitorCommand EOF virsh -q -c $test_url qemu-monitor-command test a >out 2>err && fail=1 test -s out && fail=1 -- 1.7.10.1
On 05/15/2012 05:30 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Add a VIR_ERR_NUMBER_LAST sentinel for virErrorNumber and replace the virErrorStr function by a VIR_ENUM_IMPL. Once this is done all callers of virRaiseErrorFull are simply passing in a constant string message with "%s" as the format arg. Thus virRaiseErrorFull can have its var-args removed.
* include/libvirt/virterror.h: Add VIR_ERR_NUMBER_LAST sentinel * src/rpc/virnetclientprogram.c, src/rpc/virnetclientstream.c: Remove format string passed to virRaiseErrorFull * src/util/virterror.c: Remove var-args from virRaiseErrorFull and replace virErrorStr with an VIR_ENUM * src/util/virterror_internal.h: Remove var-args from virRaiseErrorFull * tests/cpuset, tests/undefine, tests/virsh-optparse: Adapt for changes in error message --- include/libvirt/virterror.h | 37 +++ src/rpc/virnetclientprogram.c | 4 +- src/rpc/virnetclientstream.c | 2 +- src/util/virterror.c | 702 +++++++++-------------------------------- src/util/virterror_internal.h | 3 +- tests/cpuset | 2 +- tests/undefine | 2 +- tests/virsh-optparse | 2 +- 8 files changed, 193 insertions(+), 561 deletions(-)
Nice size ratio!
+++ b/include/libvirt/virterror.h @@ -172,27 +172,37 @@ typedef enum { VIR_ERR_NO_MEMORY = 2, /* memory allocation failure */ VIR_ERR_NO_SUPPORT = 3, /* no support for this function */ VIR_ERR_UNKNOWN_HOST = 4, /* could not resolve hostname */ + +
Why 2 blanks per gap instead of 1?
+VIR_ENUM_DECL(virErrorNumber) +VIR_ENUM_IMPL(virErrorNumber, VIR_ERR_NUMBER_LAST, + N_(""), /* 0 */ + N_("internal error"), + N_("out of memory"), + N_("this function is not supported by the current driver"), + N_("unknown hostname"), + + N_("no connection driver available"), /* 5 */ + N_("invalid connection pointer"),
Are you SURE that all of the new messages still make sense? I'm worried that you need to split this into some prerequisite patches that change the message contents _and all callers_, before the patch that collapses things into a VIR_ENUM. Using VIR_ERR_INVALID_CONN as an example, libvirt.c:virConnectClose calls: virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__) Pre-patch, this results in 'location: custom message': virConnectClose: invalid connection pointer in virConnectClose post-patch, this results in 'locatoin: category: half a custom message': virConnectClose: invalid connection pointer: virConnectClose and we have a bad error message. But if we first clean up all callers of VIR_ERR_INVALID_CONN to pass a useful message, rather than the current status quo of just a function name, _then_ shortening the error string to 'location: category: message' will make more sense. I did not fully audit all the messages to see which ones are the worst offenders, but I don't think I can ack this patch until we are sure we addressed the issue of bad messages first. But a quick glance shows other potential offenders (basically, anywhere an old message was not in the form 'category: %s'):
- case VIR_ERR_NO_ROOT: - if (info == NULL) - errmsg = _("missing root device information"); - else - errmsg = _("missing root device information in %s"); - break;
- case VIR_ERR_DOM_EXIST: - if (info == NULL) - errmsg = _("this domain exists already"); - else - errmsg = _("domain %s exists already"); - break; - case VIR_ERR_OPERATION_DENIED: - if (info == NULL) - errmsg = _("operation forbidden for read only access"); - else - errmsg = _("operation %s forbidden for read only access"); - break;
- case VIR_ERR_SYSTEM_ERROR: - if (info == NULL) - errmsg = _("system call error"); - else - errmsg = "%s"; - break; - case VIR_ERR_RPC: - if (info == NULL) - errmsg = _("RPC error"); - else - errmsg = "%s"; - break; - case VIR_ERR_GNUTLS_ERROR: - if (info == NULL) - errmsg = _("GNUTLS call error"); - else - errmsg = "%s"; - break;
@@ -1196,23 +780,41 @@ void virReportErrorHelper(int domcode, const char *fmt, ...) { int save_errno = errno; - va_list args; - char errorMessage[1024]; - const char *virerr; + char *msg = NULL; + char *fmtmsg = NULL; + const char *virerr = gettext(virErrorNumberTypeToString(errorcode)); + + /* Special case a couple of codes where we don't want to + * prepend some worthless generic message, to a detailed + * piece of info */ + if ((errorcode == VIR_ERR_GNUTLS_ERROR || + errorcode == VIR_ERR_RPC || + errorcode == VIR_ERR_SYSTEM_ERROR) + && fmt) + virerr = NULL;
Okay, so I see you tried to take care of some of these, at any rate.
@@ -1307,11 +904,10 @@ void virReportOOMErrorFull(int domcode, size_t linenr) { const char *virerr; - - virerr = virErrorMsg(VIR_ERR_NO_MEMORY, NULL); + virerr = gettext(virErrorNumberTypeToString(VIR_ERR_NO_MEMORY));
Why are you spelling this out as gettext() instead of using _()? I like the general idea of the patch, though. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, May 17, 2012 at 08:39:55AM -0600, Eric Blake wrote:
+VIR_ENUM_DECL(virErrorNumber) +VIR_ENUM_IMPL(virErrorNumber, VIR_ERR_NUMBER_LAST, + N_(""), /* 0 */ + N_("internal error"), + N_("out of memory"), + N_("this function is not supported by the current driver"), + N_("unknown hostname"), + + N_("no connection driver available"), /* 5 */ + N_("invalid connection pointer"),
Are you SURE that all of the new messages still make sense? I'm worried that you need to split this into some prerequisite patches that change the message contents _and all callers_, before the patch that collapses things into a VIR_ENUM. Using VIR_ERR_INVALID_CONN as an example,
libvirt.c:virConnectClose calls: virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__)
Pre-patch, this results in 'location: custom message':
virConnectClose: invalid connection pointer in virConnectClose
post-patch, this results in 'locatoin: category: half a custom message':
virConnectClose: invalid connection pointer: virConnectClose
and we have a bad error message. But if we first clean up all callers of VIR_ERR_INVALID_CONN to pass a useful message, rather than the current status quo of just a function name, _then_ shortening the error string to 'location: category: message' will make more sense.
I have just start doing this *huge* job, and I'm really liking the results. See work in progress: https://www.redhat.com/archives/libvir-list/2012-May/msg01231.html 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 :|
participants (2)
-
Daniel P. Berrange -
Eric Blake