[PATCH 0 of 2] Improve error reporting
This set adds a function to help us report more rich error messages, where appropriate. I've added in a couple that I think are critical, but would like to hear suggestions for where else we need to include the libvirt error in the CIM status.
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1227550535 28800 # Node ID af1c552b33cb473ed7ce52ddb6eb97b39b69b001 # Parent 598515fe705be5738226bb0fe789d7645769e4f3 Add vir_set_status() to misc_util.h This function allows you to set the message of a CMPIStatus object to something like what perror() would do. You give it an error message and (optionally) a virConnectPtr object and it formats it as "YourError: VirtError". It attempts to extract the last error from the connection pointer if supplied, otherwise it takes the last error in the global library pointer. I tried to make this function as foolproof as possible, providing no real error paths. It attempts to get as much info as it can into the status object, despite any failures along the way. Since this is usually to be used for error handling scenarios, this seems like the only sane approach. Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 598515fe705b -r af1c552b33cb libxkutil/misc_util.c --- a/libxkutil/misc_util.c Fri Nov 21 15:08:33 2008 -0800 +++ b/libxkutil/misc_util.c Mon Nov 24 10:15:35 2008 -0800 @@ -24,6 +24,7 @@ #include <string.h> #include <stdlib.h> #include <stdbool.h> +#include <stdarg.h> #include <unistd.h> #include <libvirt/libvirt.h> #include <libvirt/virterror.h> @@ -607,6 +608,51 @@ return vref; } +int virt_set_status(const CMPIBroker *broker, + CMPIStatus *s, + CMPIrc rc, + virConnectPtr conn, + const char *fmt, ...) +{ + va_list ap; + char *formatted = NULL; + char *verrmsg = NULL; + int ret; + virErrorPtr virt_error; + + if (conn == NULL) + virt_error = virGetLastError(); + else + virt_error = virConnGetLastError(conn); + + if (virt_error == NULL) { + if (asprintf(&verrmsg, "(none)") == -1) + verrmsg = NULL; + } else if (virt_error->message != NULL) { + verrmsg = strdup(virt_error->message); + } else { + if (asprintf(&verrmsg, "Error %i", virt_error->code) == -1) + verrmsg = NULL; + } + + va_start(ap, fmt); + ret = vasprintf(&formatted, fmt, ap); + va_end(ap); + + if (ret == -1) { + CU_DEBUG("Failed to format message for %s", fmt); + formatted = NULL; + } + + ret = cu_statusf(broker, s, rc, "%s: %s", formatted, verrmsg); + + free(formatted); + free(verrmsg); + + return ret; +} + + /* * Local Variables: * mode: C diff -r 598515fe705b -r af1c552b33cb libxkutil/misc_util.h --- a/libxkutil/misc_util.h Fri Nov 21 15:08:33 2008 -0800 +++ b/libxkutil/misc_util.h Mon Nov 24 10:15:35 2008 -0800 @@ -130,6 +130,12 @@ const CMPIObjectPath *ref, struct std_assoc_info *info); +int virt_set_status(const CMPIBroker *broker, + CMPIStatus *s, + CMPIrc rc, + virConnectPtr conn, + const char *fmt, ...); + #define LIBVIRT_CIM_DEFAULT_MAKEREF() \ static CMPIInstance* make_ref(const CMPIObjectPath *source_ref, \ const CMPIInstance *target_inst, \
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1227551693 28800 # Node ID eb2994b2c5f955067d087b1a404d025351276a70 # Parent af1c552b33cb473ed7ce52ddb6eb97b39b69b001 Make DefineSystem() and RequestStateChange() provide rich errors This patch adds use of the new virt_set_status() function to a couple of providers where it's likely to be the most effective. A DefineSystem() call can fail for a variety of reasons, as can an attempt to change the state of a guest. By pulling in the libvirt error message, we're much more likely to be able to debug the issue from just the error message the CIM client gets. Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r af1c552b33cb -r eb2994b2c5f9 src/Virt_ComputerSystem.c --- a/src/Virt_ComputerSystem.c Mon Nov 24 10:15:35 2008 -0800 +++ b/src/Virt_ComputerSystem.c Mon Nov 24 10:34:53 2008 -0800 @@ -771,27 +771,39 @@ /* This composite operation may be supported as a flag to reboot */ -static int domain_reset(virDomainPtr dom) +static CMPIStatus domain_reset(virDomainPtr dom) { int ret; virConnectPtr conn = NULL; char *xml = NULL; + CMPIStatus s = {CMPI_RC_OK, NULL}; conn = virDomainGetConnect(dom); if (conn == NULL) { CU_DEBUG("Unable to get connection from domain"); - return 1; + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to get domain connection"); + return s; } xml = virDomainGetXMLDesc(dom, 0); if (xml == NULL) { CU_DEBUG("Unable to retrieve domain XML"); - return 1; + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to get domain definition"); + return s; } ret = virDomainDestroy(dom); - if (ret) + if (ret != 0) { + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to destroy domain"); goto out; + } dom = virDomainLookupByName(virDomainGetConnect(dom), virDomainGetName(dom)); @@ -799,8 +811,11 @@ if (dom == NULL) { dom = virDomainDefineXML(conn, xml); if (dom == NULL) { - CU_DEBUG("Failed to define domain from XML"); - ret = 1; + CU_DEBUG("Failed to define domain from XML"); + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to define domain"); goto out; } } @@ -809,11 +824,16 @@ CU_DEBUG("Guest is now offline"); ret = virDomainCreate(dom); + if (ret != 0) + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Failed to start domain"); out: free(xml); - return ret; + return s; } static CMPIStatus start_domain(virDomainPtr dom) @@ -828,9 +848,10 @@ } if (virDomainCreate(dom) != 0) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Unable to start domain"); + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to start domain"); return s; } @@ -842,7 +863,6 @@ static CMPIStatus state_change_enable(virDomainPtr dom, virDomainInfoPtr info) { CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret = 0; switch (info->state) { case VIR_DOMAIN_SHUTOFF: @@ -851,7 +871,11 @@ break; case VIR_DOMAIN_PAUSED: CU_DEBUG("Unpause domain"); - ret = virDomainResume(dom); + if (virDomainResume(dom) != 0) + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to unpause domain"); break; default: CU_DEBUG("Cannot go to enabled state from %i", info->state); @@ -860,18 +884,12 @@ "Invalid state transition"); }; - if (ret != 0) - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Domain Operation Failed"); - return s; } static CMPIStatus state_change_disable(virDomainPtr dom, virDomainInfoPtr info) { CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret = 0; info->state = adjust_state_xen(dom, info->state); @@ -879,7 +897,11 @@ case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_BLOCKED: CU_DEBUG("Stop domain"); - ret = virDomainShutdown(dom); + if (virDomainShutdown(dom) != 0) + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to stop domain"); break; default: CU_DEBUG("Cannot go to disabled/shutdown state from %i", @@ -889,18 +911,12 @@ "Invalid state transition"); }; - if (ret != 0) - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Domain Operation Failed"); - return s; } static CMPIStatus state_change_pause(virDomainPtr dom, virDomainInfoPtr info) { CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret = 0; info->state = adjust_state_xen(dom, info->state); @@ -908,7 +924,11 @@ case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_BLOCKED: CU_DEBUG("Pause domain"); - ret = virDomainSuspend(dom); + if (virDomainSuspend(dom) != 0) + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to pause domain"); break; default: cu_statusf(_BROKER, &s, @@ -916,18 +936,12 @@ "Domain not running"); }; - if (ret != 0) - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Domain Operation Failed"); - return s; } static CMPIStatus state_change_reboot(virDomainPtr dom, virDomainInfoPtr info) { CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret = 0; info->state = adjust_state_xen(dom, info->state); @@ -936,7 +950,11 @@ case VIR_DOMAIN_BLOCKED: case VIR_DOMAIN_PAUSED: CU_DEBUG("Reboot domain"); - ret = virDomainReboot(dom, 0); + if (virDomainReboot(dom, 0) != 0) + virt_set_status(_BROKER, &s, + CMPI_RC_ERR_FAILED, + virDomainGetConnect(dom), + "Unable to reboot domain"); break; default: cu_statusf(_BROKER, &s, @@ -944,18 +962,12 @@ "Domain not running"); }; - if (ret != 0) - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Domain Operation Failed"); - return s; } static CMPIStatus state_change_reset(virDomainPtr dom, virDomainInfoPtr info) { CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret = 0; info->state = adjust_state_xen(dom, info->state); @@ -964,18 +976,13 @@ case VIR_DOMAIN_BLOCKED: case VIR_DOMAIN_PAUSED: CU_DEBUG("Reset domain"); - ret = domain_reset(dom); + s = domain_reset(dom); break; default: cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, "Domain not running"); }; - - if (ret != 0) - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Domain Operation Failed"); return s; } diff -r af1c552b33cb -r eb2994b2c5f9 src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c Mon Nov 24 10:15:35 2008 -0800 +++ b/src/Virt_VirtualSystemManagementService.c Mon Nov 24 10:34:53 2008 -0800 @@ -831,9 +831,10 @@ dom = virDomainDefineXML(conn, xml); if (dom == NULL) { CU_DEBUG("Failed to define domain from XML"); - cu_statusf(_BROKER, s, - CMPI_RC_ERR_FAILED, - "Failed to create domain"); + virt_set_status(_BROKER, s, + CMPI_RC_ERR_FAILED, + conn, + "Failed to define domain"); return NULL; }
Dan Smith wrote:
This set adds a function to help us report more rich error messages, where appropriate. I've added in a couple that I think are critical, but would like to hear suggestions for where else we need to include the libvirt error in the CIM status.
Very useful addition. +1 I think this could be used in device_parsing.c: _change_device() change_memory() change_vcpus() Didn't we discuss using virGetLastError() / virConnGetLastError(conn) to determine whether we should return an error that indicates a failure versus a error that indicates the given action isn't supported by libvirt? -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
KR> I think this could be used in device_parsing.c: KR> _change_device() KR> change_memory() KR> change_vcpus() Yeah, agreed. KR> Didn't we discuss using virGetLastError() / KR> virConnGetLastError(conn) to determine whether we should return an KR> error that indicates a failure versus a error that indicates the KR> given action isn't supported by libvirt? Well, it will certainly help a human client make that sort of determination. However, I'm not sure it will be useful in practice since you wouldn't expect the client to parse the error string. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (2)
-
Dan Smith -
Kaitlin Rupert