[libvirt] [PATCH] Unify most error reporting (ver 2)
The attached patch is a more complete cut at unifying error reporting among the different source files. Most files have their own custom Error function, each with varying degrees of functionality, and lots of code duplication. The attached patch adds a helper function to virterror.c that centralizes all this logic, and redefines each local error function as a macro that calls out to the helper. The fixed files now offer printf style reporting, and the macros pass off function name, file name, and line number of the reported error (currently not used, but handy to have). This change centralizes probably 90% of the error calls. Some files have pretty custom error functions that don't map easily to the helper, or call __virRaiseError directly in a number of places, so I skipped these for now. Eventually we should move all these edge cases over so any src/ wide error changes will be pretty easy to make. The one thing we lose here is that some some places were logging info in the err{1,2,3} and int{1,2} fields of raised error. I don't consider this a loss: even where it was used, it was used inconsistently and rarely for anything that would be useful to a user. In the places where the data had some value, I included it in the error string. One side question: is src/xmlrpc.{c,h} even used? And if not, can it be removed? Thanks, Cole domain_conf.c | 27 +----------- hash.c | 26 +----------- internal.h | 6 ++ lxc_conf.c | 21 --------- lxc_conf.h | 7 +-- network_conf.c | 23 +--------- openvz_conf.c | 22 ---------- openvz_conf.h | 7 ++- proxy_internal.c | 23 +--------- qemu_conf.c | 21 --------- qemu_conf.h | 8 +-- qparams.c | 10 +--- sexpr.c | 25 +---------- storage_conf.c | 19 --------- storage_conf.h | 7 +-- test.c | 28 ++++--------- util.c | 27 +----------- virterror.c | 40 ++++++++++++++++++ xen_internal.c | 116 ++++++++++++++++++------------------------------------- xen_unified.c | 24 ++--------- xend_internal.c | 59 ++------------------------- xm_internal.c | 28 ++----------- xml.c | 43 +++++--------------- xs_internal.c | 23 +--------- 24 files changed, 164 insertions(+), 476 deletions(-)
On Wed, Oct 08, 2008 at 02:17:45PM -0400, Cole Robinson wrote:
The attached patch is a more complete cut at unifying error reporting among the different source files. Most files have their own custom Error function, each with varying degrees of functionality, and lots of code duplication.
The attached patch adds a helper function to virterror.c that centralizes all this logic, and redefines each local error function as a macro that calls out to the helper. The fixed files now offer printf style reporting, and the macros pass off function name, file name, and line number of the reported error (currently not used, but handy to have).
This change centralizes probably 90% of the error calls. Some files have pretty custom error functions that don't map easily to the helper, or call __virRaiseError directly in a number of places, so I skipped these for now. Eventually we should move all these edge cases over so any src/ wide error changes will be pretty easy to make.
Ok.
The one thing we lose here is that some some places were logging info in the err{1,2,3} and int{1,2} fields of raised error. I don't consider this a loss: even where it was used, it was used inconsistently and rarely for anything that would be useful to a user. In the places where the data had some value, I included it in the error string.
Agreed - it had no documented use either.
One side question: is src/xmlrpc.{c,h} even used? And if not, can it be removed?
It is not used at this time. It was for the hypothetical alternative Xen driver which talks XML-RPC which we never got around to writing. Perhaps it really is time to kill it -we can get it back from CVS if ever required. ACK to this patch Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
One side question: is src/xmlrpc.{c,h} even used? And if not, can it be removed?
It is not used at this time. It was for the hypothetical alternative Xen driver which talks XML-RPC which we never got around to writing. Perhaps it really is time to kill it -we can get it back from CVS if ever required.
Agreed,
ACK to this patch
+1 too, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
One side question: is src/xmlrpc.{c,h} even used? And if not, can it be removed? It is not used at this time. It was for the hypothetical alternative Xen driver which talks XML-RPC which we never got around to writing. Perhaps it really is time to kill it -we can get it back from CVS if ever required.
Agreed,
ACK to this patch
+1 too,
Daniel
Thanks, pushed now. - Cole
On Thu, Oct 09, 2008 at 11:44:02AM -0400, Cole Robinson wrote:
Daniel Veillard wrote:
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
One side question: is src/xmlrpc.{c,h} even used? And if not, can it be removed? It is not used at this time. It was for the hypothetical alternative Xen driver which talks XML-RPC which we never got around to writing. Perhaps it really is time to kill it -we can get it back from CVS if ever required.
Agreed,
ACK to this patch
+1 too,
Daniel
Thanks, pushed now.
Hum, this breaks "make check" PASS: read-non-seekable --- out 2008-10-10 10:23:57.000000000 +0200 +++ exp 2008-10-10 10:23:57.000000000 +0200 @@ -1,2 +1,2 @@ -libvir: Test error : internal error Domain is still running +libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test FAIL: undefine Seems we loose the domain name in the error message when trying to undefine a running domain: paphio:~/libvirt/tests -> /usr/bin/virsh -q -c test:///default undefine test libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests -> ../src/virsh -q -c test:///default undefine test libvir: Test error : internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests -> I should be possible to restore the old behaviour of reporting the domain name there Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Oct 09, 2008 at 11:44:02AM -0400, Cole Robinson wrote:
Daniel Veillard wrote:
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
One side question: is src/xmlrpc.{c,h} even used? And if not, can it be removed? It is not used at this time. It was for the hypothetical alternative Xen driver which talks XML-RPC which we never got around to writing. Perhaps it really is time to kill it -we can get it back from CVS if ever required.
Agreed,
ACK to this patch
+1 too,
Daniel
Thanks, pushed now.
Hum, this breaks "make check"
PASS: read-non-seekable --- out 2008-10-10 10:23:57.000000000 +0200 +++ exp 2008-10-10 10:23:57.000000000 +0200 @@ -1,2 +1,2 @@ -libvir: Test error : internal error Domain is still running +libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test FAIL: undefine
Seems we loose the domain name in the error message when trying to undefine a running domain:
paphio:~/libvirt/tests -> /usr/bin/virsh -q -c test:///default undefine test libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests -> ../src/virsh -q -c test:///default undefine test libvir: Test error : internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests ->
I should be possible to restore the old behaviour of reporting the domain name there
The documentation suggests that the new interface was initially supposed to have dom and net parameters: /** * __virReportErrorHelper * * @conn: the connection to the hypervisor if available * @dom: the domain if available * @net: the network if available * @domcode: the virErrorDomain indicating where it's coming from ... void __virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, const char *filename ATTRIBUTE_UNUSED, const char *funcname ATTRIBUTE_UNUSED, long long linenr ATTRIBUTE_UNUSED, const char *fmt, ...) { ... virerr = __virErrorMsg(errcode, (errorMessage[0] ? errorMessage : NULL)); __virRaiseError(conn, NULL, NULL, domcode, errcode, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); } Shouldn't be hard to make the code agree with the documentation. There are fewer than 20 uses of __virReportErrorHelper so far. In fact, I've just done it. While doing it, I found that there were a few other places where dom- and net-info would have been lost. Along the way, there was a macro that asked to be wrapped with the classic do {...} while(0), so I obliged. Here's the patch. With it, "make check" passes once again. Adjust __virReportErrorHelper to accept "dom" and "net", parameters, since some callers require passing those to __virRaiseError. * src/virterror.c (__virReportErrorHelper): Add dom and net parameters, to match documentation. * src/internal.h: Update its prototype. * src/domain_conf.c (virDomainReportError): Update caller. * src/hash.c (virHashError): Likewise. * src/lxc_conf.h (lxcError): Likewise. * src/network_conf.c (virNetworkReportError): Likewise. * src/openvz_conf.h (openvzError): Likewise. * src/proxy_internal.c (virProxyError): Likewise. * src/qemu_conf.h (qemudReportError): Likewise. * src/qparams.c (qparam_report_oom): Likewise. * src/sexpr.c (virSexprError): Likewise. * src/storage_conf.h (virStorageReportError): Likewise. * src/test.c (testError): Likewise. * src/util.c (ReportError): Likewise. * src/xen_internal.c (virXenError): Likewise. * src/xen_unified.c (xenUnifiedError): Likewise. * src/xend_internal.c (virXendError): Likewise. * src/xm_internal.c (xenXMError): Likewise. * src/xml.c (virXMLError): Likewise. * src/xs_internal.c (virXenStoreError): Likewise.
From 63ab950ba9b8065de3036dcf502e14c05c6f6461 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 10 Oct 2008 11:45:16 +0200 Subject: [PATCH] Adjust __virReportErrorHelper to accept "dom" and "net"
parameters, since some callers require passing those to __virRaiseError. * src/virterror.c (__virReportErrorHelper): Add dom and net parameters, to match documentation. * src/internal.h: Update its prototype. * src/domain_conf.c (virDomainReportError): Update caller. * src/hash.c (virHashError): Likewise. * src/lxc_conf.h (lxcError): Likewise. * src/network_conf.c (virNetworkReportError): Likewise. * src/openvz_conf.h (openvzError): Likewise. * src/proxy_internal.c (virProxyError): Likewise. * src/qemu_conf.h (qemudReportError): Likewise. * src/qparams.c (qparam_report_oom): Likewise. * src/sexpr.c (virSexprError): Likewise. * src/storage_conf.h (virStorageReportError): Likewise. * src/test.c (testError): Likewise. * src/util.c (ReportError): Likewise. * src/xen_internal.c (virXenError): Likewise. * src/xen_unified.c (xenUnifiedError): Likewise. * src/xend_internal.c (virXendError): Likewise. * src/xm_internal.c (xenXMError): Likewise. * src/xml.c (virXMLError): Likewise. * src/xs_internal.c (virXenStoreError): Likewise. --- src/domain_conf.c | 6 +++--- src/hash.c | 6 +++--- src/internal.h | 7 +++++-- src/lxc_conf.h | 7 +++---- src/network_conf.c | 6 +++--- src/openvz_conf.h | 6 +++--- src/proxy_internal.c | 6 +++--- src/qemu_conf.h | 8 ++++---- src/qparams.c | 9 +++++---- src/sexpr.c | 6 +++--- src/storage_conf.h | 6 +++--- src/test.c | 6 +++--- src/util.c | 6 +++--- src/virterror.c | 9 ++++++--- src/xen_internal.c | 10 ++++++---- src/xen_unified.c | 6 +++--- src/xend_internal.c | 6 +++--- src/xm_internal.c | 6 +++--- src/xml.c | 6 +++--- src/xs_internal.c | 8 ++++---- 20 files changed, 72 insertions(+), 64 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index e8d248f..9e5482f 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -141,9 +141,9 @@ VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, "usb", "pci") -#define virDomainReportError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_DOMAIN, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virDomainReportError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_DOMAIN, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) virDomainObjPtr virDomainFindByID(const virDomainObjPtr doms, int id) diff --git a/src/hash.c b/src/hash.c index 3efda2c..a395d58 100644 --- a/src/hash.c +++ b/src/hash.c @@ -31,9 +31,9 @@ /* #define DEBUG_GROW */ -#define virHashError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_NONE, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virHashError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_NONE, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /* * A single entry in the hash table diff --git a/src/internal.h b/src/internal.h index 2ae764d..04d4d3c 100644 --- a/src/internal.h +++ b/src/internal.h @@ -313,12 +313,15 @@ void __virRaiseError(virConnectPtr conn, int int1, int int2, const char *msg, ...) ATTRIBUTE_FORMAT(printf, 12, 13); const char *__virErrorMsg(virErrorNumber error, const char *info); -void __virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, +void __virReportErrorHelper(virConnectPtr conn, + virDomainPtr dom, + virNetworkPtr net, + int domcode, int errcode, const char *filename ATTRIBUTE_UNUSED, const char *funcname ATTRIBUTE_UNUSED, long long linenr ATTRIBUTE_UNUSED, const char *fmt, ...) - ATTRIBUTE_FORMAT(printf, 7, 8); + ATTRIBUTE_FORMAT(printf, 9, 10); /************************************************************************ * * diff --git a/src/lxc_conf.h b/src/lxc_conf.h index 4d57046..b15ef5c 100644 --- a/src/lxc_conf.h +++ b/src/lxc_conf.h @@ -49,9 +49,8 @@ struct __lxc_driver { int lxcLoadDriverConfig(lxc_driver_t *driver); virCapsPtr lxcCapsInit(void); -#define lxcError(conn, dom, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_LXC, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define lxcError(conn, dom, code, fmt...) \ + __virReportErrorHelper(conn, dom, NULL, VIR_FROM_LXC, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) #endif /* LXC_CONF_H */ - diff --git a/src/network_conf.c b/src/network_conf.c index 800ead9..3b9100e 100644 --- a/src/network_conf.c +++ b/src/network_conf.c @@ -48,9 +48,9 @@ VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route" ) -#define virNetworkReportError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_NETWORK, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virNetworkReportError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_NETWORK, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) virNetworkObjPtr virNetworkFindByUUID(const virNetworkObjPtr nets, const unsigned char *uuid) diff --git a/src/openvz_conf.h b/src/openvz_conf.h index fbe4f69..828d807 100644 --- a/src/openvz_conf.h +++ b/src/openvz_conf.h @@ -41,9 +41,9 @@ enum { OPENVZ_WARN, OPENVZ_ERR }; fprintf(stderr, msg);\ fprintf(stderr, "\n"); } -#define openvzError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_OPENVZ, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define openvzError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_OPENVZ, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /* OpenVZ commands - Replace with wrapper scripts later? */ diff --git a/src/proxy_internal.c b/src/proxy_internal.c index 0186eba..2cb1862 100644 --- a/src/proxy_internal.c +++ b/src/proxy_internal.c @@ -92,9 +92,9 @@ struct xenUnifiedDriver xenProxyDriver = { * * ************************************************************************/ -#define virProxyError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_PROXY, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virProxyError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_PROXY, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /************************************************************************ * * diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 5ea57f0..75d17de 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -1,7 +1,7 @@ /* * config.h: VM configuration management * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -71,9 +71,9 @@ struct qemud_driver { }; -#define qemudReportError(conn, dom, net, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_QEMU, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define qemudReportError(conn, dom, net, code, fmt...) \ + __virReportErrorHelper(conn, dom, net, VIR_FROM_QEMU, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) int qemudLoadDriverConfig(struct qemud_driver *driver, diff --git a/src/qparams.c b/src/qparams.c index ed5bddc..2628163 100644 --- a/src/qparams.c +++ b/src/qparams.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2007 Red Hat, Inc. +/* Copyright (C) 2007, 2008 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -30,9 +30,10 @@ #include "memory.h" #include "qparams.h" -#define qparam_report_oom(void) \ - __virReportErrorHelper(NULL, VIR_FROM_NONE, VIR_ERR_NO_MEMORY, \ - __FILE__, __FUNCTION__, __LINE__, NULL) +#define qparam_report_oom(void) \ + __virReportErrorHelper(NULL, NULL, NULL, VIR_FROM_NONE, \ + VIR_ERR_NO_MEMORY, \ + __FILE__, __FUNCTION__, __LINE__, NULL) struct qparam_set * new_qparam_set (int init_alloc, ...) diff --git a/src/sexpr.c b/src/sexpr.c index c168754..f8eb39a 100644 --- a/src/sexpr.c +++ b/src/sexpr.c @@ -23,9 +23,9 @@ #include "util.h" #include "memory.h" -#define virSexprError(code, fmt...) \ - __virReportErrorHelper(NULL, VIR_FROM_SEXPR, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virSexprError(code, fmt...) \ + __virReportErrorHelper(NULL, NULL, NULL, VIR_FROM_SEXPR, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /** * sexpr_new: diff --git a/src/storage_conf.h b/src/storage_conf.h index c86bf4b..599b5e3 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -247,9 +247,9 @@ static inline int virStoragePoolObjIsActive(virStoragePoolObjPtr pool) { return pool->active; } -#define virStorageReportError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_STORAGE, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virStorageReportError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_STORAGE, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) int virStoragePoolObjScanConfigs(virStorageDriverStatePtr driver); diff --git a/src/test.c b/src/test.c index ad51736..7fd2145 100644 --- a/src/test.c +++ b/src/test.c @@ -114,9 +114,9 @@ static const virNodeInfo defaultNodeInfo = { privconn = (testConnPtr)conn->privateData; -#define testError(conn, dom, net, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_TEST, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define testError(conn, dom, net, code, fmt...) \ + __virReportErrorHelper(conn, dom, net, VIR_FROM_TEST, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) static virCapsPtr testBuildCapabilities(virConnectPtr conn) { diff --git a/src/util.c b/src/util.c index ebe86b2..bb6dc9b 100644 --- a/src/util.c +++ b/src/util.c @@ -66,9 +66,9 @@ #ifndef PROXY -#define ReportError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_NONE, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define ReportError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_NONE, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) int virFileStripSuffix(char *str, const char *suffix) diff --git a/src/virterror.c b/src/virterror.c index 21c7339..22de342 100644 --- a/src/virterror.c +++ b/src/virterror.c @@ -1,7 +1,7 @@ /* * virterror.c: implements error handling and reporting code for libvirt * - * Copy: Copyright (C) 2006 Red Hat, Inc. + * Copy: Copyright (C) 2006, 2008 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -740,7 +740,10 @@ __virErrorMsg(virErrorNumber error, const char *info) * Helper function to do most of the grunt work for individual driver * ReportError */ -void __virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, +void __virReportErrorHelper(virConnectPtr conn, + virDomainPtr dom, + virNetworkPtr net, + int domcode, int errcode, const char *filename ATTRIBUTE_UNUSED, const char *funcname ATTRIBUTE_UNUSED, long long linenr ATTRIBUTE_UNUSED, @@ -759,7 +762,7 @@ void __virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, } virerr = __virErrorMsg(errcode, (errorMessage[0] ? errorMessage : NULL)); - __virRaiseError(conn, NULL, NULL, domcode, errcode, VIR_ERR_ERROR, + __virRaiseError(conn, dom, net, domcode, errcode, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); } diff --git a/src/xen_internal.c b/src/xen_internal.c index f498cc3..3e223cb 100644 --- a/src/xen_internal.c +++ b/src/xen_internal.c @@ -717,10 +717,12 @@ struct xenUnifiedDriver xenHypervisorDriver = { }; #endif /* !PROXY */ -#define virXenError(conn, code, fmt...) \ - if (in_init == 0) \ - __virReportErrorHelper(conn, VIR_FROM_XEN, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virXenError(conn, code, fmt...) \ + do { \ + if (in_init == 0) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_XEN, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt); \ + } while (0) #ifndef PROXY diff --git a/src/xen_unified.c b/src/xen_unified.c index 016181e..884a29d 100644 --- a/src/xen_unified.c +++ b/src/xen_unified.c @@ -58,9 +58,9 @@ static struct xenUnifiedDriver *drivers[XEN_UNIFIED_NR_DRIVERS] = { [XEN_UNIFIED_XM_OFFSET] = &xenXMDriver, }; -#define xenUnifiedError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_XEN, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define xenUnifiedError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_XEN, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /* * Helper functions currently used in the NUMA code diff --git a/src/xend_internal.c b/src/xend_internal.c index 2d10e11..aacf8db 100644 --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -104,9 +104,9 @@ virDomainXMLDevID(virDomainPtr domain, int ref_len); #endif -#define virXendError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_XEND, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virXendError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_XEND, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) #define virXendErrorInt(conn, code, ival) \ virXendError(conn, code, "%d", ival) diff --git a/src/xm_internal.c b/src/xm_internal.c index a2dd9aa..1b783f9 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -124,9 +124,9 @@ struct xenUnifiedDriver xenXMDriver = { NULL, /* domainSetSchedulerParameters */ }; -#define xenXMError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_XENXM, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define xenXMError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_XENXM, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) int xenXMInit (void) diff --git a/src/xml.c b/src/xml.c index 6e6e0f6..a0cfdd7 100644 --- a/src/xml.c +++ b/src/xml.c @@ -22,9 +22,9 @@ #include "util.h" #include "memory.h" -#define virXMLError(conn, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_XML, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virXMLError(conn, code, fmt...) \ + __virReportErrorHelper(conn, NULL, NULL, VIR_FROM_XML, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /************************************************************************ diff --git a/src/xs_internal.c b/src/xs_internal.c index fce7fa7..3f796ad 100644 --- a/src/xs_internal.c +++ b/src/xs_internal.c @@ -1,7 +1,7 @@ /* * xs_internal.c: access to Xen Store * - * Copyright (C) 2006 Red Hat, Inc. + * Copyright (C) 2006, 2008 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -87,9 +87,9 @@ struct xenUnifiedDriver xenStoreDriver = { #endif /* ! PROXY */ -#define virXenStoreError(conn, code, fmt...) \ - __virReportErrorHelper(NULL, VIR_FROM_XENSTORE, code, __FILE__, \ - __FUNCTION__, __LINE__, fmt) +#define virXenStoreError(conn, code, fmt...) \ + __virReportErrorHelper(NULL, NULL, NULL, VIR_FROM_XENSTORE, code, \ + __FILE__, __FUNCTION__, __LINE__, fmt) /************************************************************************ * * -- 1.6.0.2.307.gc4275
On Fri, Oct 10, 2008 at 11:47:49AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Oct 09, 2008 at 11:44:02AM -0400, Cole Robinson wrote:
Daniel Veillard wrote:
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
Hum, this breaks "make check"
PASS: read-non-seekable --- out 2008-10-10 10:23:57.000000000 +0200 +++ exp 2008-10-10 10:23:57.000000000 +0200 @@ -1,2 +1,2 @@ -libvir: Test error : internal error Domain is still running +libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test FAIL: undefine
Seems we loose the domain name in the error message when trying to undefine a running domain:
paphio:~/libvirt/tests -> /usr/bin/virsh -q -c test:///default undefine test libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests -> ../src/virsh -q -c test:///default undefine test libvir: Test error : internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests ->
I should be possible to restore the old behaviour of reporting the domain name there
The documentation suggests that the new interface was initially supposed to have dom and net parameters:
We removed them because they're deprecated and 90% of the driver code never provides them - the test driver was the exception to the rule here
/** * __virReportErrorHelper * * @conn: the connection to the hypervisor if available * @dom: the domain if available * @net: the network if available * @domcode: the virErrorDomain indicating where it's coming from ... void __virReportErrorHelper(virConnectPtr conn, int domcode, int errcode, const char *filename ATTRIBUTE_UNUSED, const char *funcname ATTRIBUTE_UNUSED, long long linenr ATTRIBUTE_UNUSED, const char *fmt, ...) { ... virerr = __virErrorMsg(errcode, (errorMessage[0] ? errorMessage : NULL)); __virRaiseError(conn, NULL, NULL, domcode, errcode, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); }
Shouldn't be hard to make the code agree with the documentation. There are fewer than 20 uses of __virReportErrorHelper so far.
That's not really solving the problem because very little code passes in a non-NULL dom/net parameter. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Oct 10, 2008 at 11:47:49AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Oct 09, 2008 at 11:44:02AM -0400, Cole Robinson wrote:
Daniel Veillard wrote:
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
Hum, this breaks "make check"
PASS: read-non-seekable --- out 2008-10-10 10:23:57.000000000 +0200 +++ exp 2008-10-10 10:23:57.000000000 +0200 @@ -1,2 +1,2 @@ -libvir: Test error : internal error Domain is still running +libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test FAIL: undefine
Seems we loose the domain name in the error message when trying to undefine a running domain:
paphio:~/libvirt/tests -> /usr/bin/virsh -q -c test:///default undefine test libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests -> ../src/virsh -q -c test:///default undefine test libvir: Test error : internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests ->
I should be possible to restore the old behaviour of reporting the domain name there
The documentation suggests that the new interface was initially supposed to have dom and net parameters:
We removed them because they're deprecated and 90% of the driver code never provides them - the test driver was the exception to the rule here
Oh well. FYI, the other exceptions: lxc_conf.h: lxcError (dom-only) qemu_conf.h: qemudReportError ...
That's not really solving the problem because very little code passes in a non-NULL dom/net parameter.
On Fri, Oct 10, 2008 at 11:59:34AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Oct 10, 2008 at 11:47:49AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Oct 09, 2008 at 11:44:02AM -0400, Cole Robinson wrote:
Daniel Veillard wrote:
On Wed, Oct 08, 2008 at 08:35:38PM +0100, Daniel P. Berrange wrote:
Hum, this breaks "make check"
PASS: read-non-seekable --- out 2008-10-10 10:23:57.000000000 +0200 +++ exp 2008-10-10 10:23:57.000000000 +0200 @@ -1,2 +1,2 @@ -libvir: Test error : internal error Domain is still running +libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test FAIL: undefine
Seems we loose the domain name in the error message when trying to undefine a running domain:
paphio:~/libvirt/tests -> /usr/bin/virsh -q -c test:///default undefine test libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests -> ../src/virsh -q -c test:///default undefine test libvir: Test error : internal error Domain is still running error: Failed to undefine domain test paphio:~/libvirt/tests ->
I should be possible to restore the old behaviour of reporting the domain name there
The documentation suggests that the new interface was initially supposed to have dom and net parameters:
We removed them because they're deprecated and 90% of the driver code never provides them - the test driver was the exception to the rule here
Oh well. FYI, the other exceptions: lxc_conf.h: lxcError (dom-only) qemu_conf.h: qemudReportError
Some parts of qemu supply it, many other parts do not since they have no access to the virDomainPtr object. This is one of the reasons for deprecating this field - it was impossible to reliably provide it when raising errors. In this particular test case error, we are better off supplying the domain name in the format string - it'll improve the error message to have it placed in context of the description eg, instead of libvir: Test error test: internal error Domain is still running We'd have libvir: Test error: internal error Domain 'test' is still running Which could be done by changing if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain is still running")); to be if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain '%s' is still running"), domain->name); We already follow this pattern throughout the QEMU driver eg vm = virDomainFindByName(&driver->domains, def->name); if (vm) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, _("domain '%s' is already defined"), def->name); goto cleanup; } eg virUUIDFormat(def->uuid, uuidstr); qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, _("domain with uuid '%s' is already defined"), uuidstr); Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Some parts of qemu supply it, many other parts do not since they have no access to the virDomainPtr object. This is one of the reasons for deprecating this field - it was impossible to reliably provide it when raising errors.
In this particular test case error, we are better off supplying the domain name in the format string - it'll improve the error message to have it placed in context of the description
eg, instead of
libvir: Test error test: internal error Domain is still running
We'd have
libvir: Test error: internal error Domain 'test' is still running
Which could be done by changing
if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain is still running"));
to be
if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain '%s' is still running"), domain->name);
Yep. I began doing that about 15 minutes ago ;-)
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Some parts of qemu supply it, many other parts do not since they have no access to the virDomainPtr object. This is one of the reasons for deprecating this field - it was impossible to reliably provide it when raising errors.
In this particular test case error, we are better off supplying the domain name in the format string - it'll improve the error message to have it placed in context of the description
eg, instead of
libvir: Test error test: internal error Domain is still running
We'd have
libvir: Test error: internal error Domain 'test' is still running
Which could be done by changing
if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain is still running"));
to be
if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain '%s' is still running"), domain->name);
Yep. I began doing that about 15 minutes ago ;-)
Here's the change to adjust src/test.c. Two change sets: - Fix the "make check" failure by adjusting uses of testError, including domain->name (and a few net->name) strings via format. - Remove now-ignored dom and net parameters.
From e6e5387e1e8f8ea7f60bf8666c188b1ee9852700 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 10 Oct 2008 14:45:42 +0200 Subject: [PATCH] change testError format strings to explicitly include domain and network names
* src/test.c (testResumeDomain, testPauseDomain): Likewise. (testShutdownDomain, testGetDomainInfo, testDomainSave): Likewise. (testSetMemory, testSetVcpus, testDomainCreate, testDomainUndefine) (testDomainGetSchedulerType, testDomainGetSchedulerParams): Likewise. (testDomainSetSchedulerParams, testNetworkUndefine): Likewise. (testNetworkStart, testNetworkGetBridgeName): Likewise. (testDomainCoreDump): Likewise. Name the file we failed to open or write. Use strerror(errno) in diagnostics. * tests/undefine: Adjust expected output. --- src/test.c | 87 ++++++++++++++++++++++++++++++------------------------- tests/undefine | 2 +- 2 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/test.c b/src/test.c index ad51736..7ca50eb 100644 --- a/src/test.c +++ b/src/test.c @@ -776,8 +776,9 @@ static int testResumeDomain (virDomainPtr domain) GET_DOMAIN(domain, -1); if (privdom->state != VIR_DOMAIN_PAUSED) { - testError(domain->conn, domain, NULL, - VIR_ERR_INTERNAL_ERROR, _("domain not paused")); + testError(domain->conn, NULL, NULL, + VIR_ERR_INTERNAL_ERROR, _("%s: domain not paused"), + domain->name); return -1; } @@ -791,8 +792,9 @@ static int testPauseDomain (virDomainPtr domain) if (privdom->state == VIR_DOMAIN_SHUTOFF || privdom->state == VIR_DOMAIN_PAUSED) { - testError(domain->conn, domain, NULL, - VIR_ERR_INTERNAL_ERROR, _("domain not running")); + testError(domain->conn, NULL, NULL, + VIR_ERR_INTERNAL_ERROR, _("%s: domain not running"), + domain->name); return -1; } @@ -805,7 +807,8 @@ static int testShutdownDomain (virDomainPtr domain) GET_DOMAIN(domain, -1); if (privdom->state == VIR_DOMAIN_SHUTOFF) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "domain not running"); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: domain not running"), domain->name); return -1; } @@ -861,7 +864,8 @@ static int testGetDomainInfo (virDomainPtr domain, GET_DOMAIN(domain, -1); if (gettimeofday(&tv, NULL) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, _("getting time of day")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("getting time of day")); return (-1); } @@ -886,40 +890,40 @@ static int testDomainSave(virDomainPtr domain, xml = testDomainDumpXML(domain, 0); if (xml == NULL) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot allocate space for metadata")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: cannot allocate space for metadata"), domain->name); return (-1); } if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot save domain")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: cannot save domain"), domain->name); return (-1); } len = strlen(xml); if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot write header")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: cannot write header"), domain->name); close(fd); return (-1); } if (safewrite(fd, (char*)&len, sizeof(len)) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot write metadata length")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: cannot write metadata length"), domain->name); close(fd); return (-1); } if (safewrite(fd, xml, len) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot write metadata")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: cannot write metadata"), domain->name); VIR_FREE(xml); close(fd); return (-1); } VIR_FREE(xml); if (close(fd) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot save domain data")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("%s: cannot save domain data"), domain->name); close(fd); return (-1); } @@ -1007,19 +1011,22 @@ static int testDomainCoreDump(virDomainPtr domain, GET_DOMAIN(domain, -1); if ((fd = open(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot save domain core")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("domain '%s' coredump: failed to open %s: %s"), + domain->name, to, strerror (errno)); return (-1); } if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot write header")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("domain '%s' coredump: failed to write header to %s: %s"), + domain->name, to, strerror (errno)); close(fd); return (-1); } if (close(fd) < 0) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("cannot save domain data")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("domain '%s' coredump: write failed: %s: %s"), + domain->name, to, strerror (errno)); close(fd); return (-1); } @@ -1060,7 +1067,7 @@ static int testSetMemory(virDomainPtr domain, GET_DOMAIN(domain, -1); if (memory > privdom->def->maxmem) { - testError(domain->conn, domain, NULL, + testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); } @@ -1075,7 +1082,7 @@ static int testSetVcpus(virDomainPtr domain, /* We allow more cpus in guest than host */ if (nrCpus > 32) { - testError(domain->conn, domain, NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); + testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); } @@ -1186,8 +1193,8 @@ static int testDomainCreate(virDomainPtr domain) { GET_DOMAIN(domain, -1); if (privdom->state != VIR_DOMAIN_SHUTOFF) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("Domain is already running")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Domain '%s' is already running"), domain->name); return (-1); } @@ -1201,8 +1208,8 @@ static int testDomainUndefine(virDomainPtr domain) { GET_DOMAIN(domain, -1); if (privdom->state != VIR_DOMAIN_SHUTOFF) { - testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, - _("Domain is still running")); + testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Domain '%s' is still running"), domain->name); return (-1); } @@ -1237,7 +1244,7 @@ static char *testDomainGetSchedulerType(virDomainPtr domain, *nparams = 1; type = strdup("fair"); if (!type) { - testError(domain->conn, domain, NULL, VIR_ERR_NO_MEMORY, "schedular"); + testError(domain->conn, NULL, NULL, VIR_ERR_NO_MEMORY, "schedular"); return (NULL); } return type; @@ -1249,7 +1256,7 @@ static int testDomainGetSchedulerParams(virDomainPtr domain, { GET_DOMAIN(domain, -1); if (*nparams != 1) { - testError(domain->conn, domain, NULL, VIR_ERR_INVALID_ARG, "nparams"); + testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "nparams"); return (-1); } strcpy(params[0].field, "weight"); @@ -1267,15 +1274,15 @@ static int testDomainSetSchedulerParams(virDomainPtr domain, { GET_DOMAIN(domain, -1); if (nparams != 1) { - testError(domain->conn, domain, NULL, VIR_ERR_INVALID_ARG, "nparams"); + testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "nparams"); return (-1); } if (STRNEQ(params[0].field, "weight")) { - testError(domain->conn, domain, NULL, VIR_ERR_INVALID_ARG, "field"); + testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "field"); return (-1); } if (params[0].type != VIR_DOMAIN_SCHED_FIELD_UINT) { - testError(domain->conn, domain, NULL, VIR_ERR_INVALID_ARG, "type"); + testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "type"); return (-1); } /* XXX */ @@ -1441,8 +1448,8 @@ static int testNetworkUndefine(virNetworkPtr network) { GET_NETWORK(network, -1); if (virNetworkIsActive(privnet)) { - testError(network->conn, NULL, network, VIR_ERR_INTERNAL_ERROR, - _("Network is still running")); + testError(network->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Network '%s' is still running"), network->name); return (-1); } @@ -1456,8 +1463,8 @@ static int testNetworkStart(virNetworkPtr network) { GET_NETWORK(network, -1); if (virNetworkIsActive(privnet)) { - testError(network->conn, NULL, network, VIR_ERR_INTERNAL_ERROR, - _("Network is already running")); + testError(network->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Network '%s' is already running"), network->name); return (-1); } @@ -1488,7 +1495,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { GET_NETWORK(network, NULL); if (privnet->def->bridge && !(bridge = strdup(privnet->def->bridge))) { - testError(network->conn, NULL, network, VIR_ERR_NO_MEMORY, "network"); + testError(network->conn, NULL, NULL, VIR_ERR_NO_MEMORY, "network"); return NULL; } return bridge; diff --git a/tests/undefine b/tests/undefine index 3936e22..d745241 100755 --- a/tests/undefine +++ b/tests/undefine @@ -29,7 +29,7 @@ fail=0 virsh -q -c test:///default undefine test > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -libvir: Test error test: internal error Domain is still running +libvir: Test error : internal error Domain 'test' is still running error: Failed to undefine domain test EOF compare out exp || fail=1 -- 1.6.0.2.307.gc4275
From 6e10cf8adc031e11adfdaf10553dc3a6b38ed0ab Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 10 Oct 2008 15:10:35 +0200 Subject: [PATCH] test.c (testError): Remove now-ignored dom and net parameters.
This is a sytnax-only change: removing the two unused parameters and updating all callers: * src/test.c (GET_DOMAIN, GET_NETWORK, testError) (testBuildCapabilities, testOpenDefault, testOpenFromFile) (testOpen, testGetHostname, testGetURI, testGetCapabilities) (testLookupDomainByID, testLookupDomainByUUID) (testLookupDomainByName, testResumeDomain, testPauseDomain) (testShutdownDomain, testGetDomainInfo, testDomainSave) (testDomainRestore, testDomainCoreDump, testGetOSType) (testSetMemory, testSetVcpus, testListDefinedDomains) (testNodeGetCellsFreeMemory, testDomainCreate) (testDomainUndefine, testDomainGetSchedulerType) (testDomainGetSchedulerParams, testDomainSetSchedulerParams) (testLookupNetworkByUUID, testLookupNetworkByName) (testListNetworks, testListDefinedNetworks, testNetworkUndefine) (testNetworkStart, testNetworkGetBridgeName): Update callers. --- src/test.c | 142 +++++++++++++++++++++++++++++------------------------------ 1 files changed, 70 insertions(+), 72 deletions(-) diff --git a/src/test.c b/src/test.c index 7ca50eb..00d5541 100644 --- a/src/test.c +++ b/src/test.c @@ -88,8 +88,7 @@ static const virNodeInfo defaultNodeInfo = { do { \ if ((privdom = virDomainFindByName(privconn->domains, \ (dom)->name)) == NULL) { \ - testError((dom)->conn, (dom), NULL, VIR_ERR_INVALID_ARG, \ - __FUNCTION__); \ + testError((dom)->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); \ return (ret); \ } \ } while (0) @@ -102,8 +101,7 @@ static const virNodeInfo defaultNodeInfo = { do { \ if ((privnet = virNetworkFindByName(privconn->networks, \ (net)->name)) == NULL) { \ - testError((net)->conn, NULL, (net), VIR_ERR_INVALID_ARG, \ - __FUNCTION__); \ + testError((net)->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); \ return (ret); \ } \ } while (0) @@ -114,8 +112,8 @@ static const virNodeInfo defaultNodeInfo = { privconn = (testConnPtr)conn->privateData; -#define testError(conn, dom, net, code, fmt...) \ - __virReportErrorHelper(conn, VIR_FROM_TEST, code, __FILE__, \ +#define testError(conn, code, fmt...) \ + __virReportErrorHelper(conn, VIR_FROM_TEST, code, __FILE__, \ __FUNCTION__, __LINE__, fmt) static virCapsPtr @@ -168,7 +166,7 @@ testBuildCapabilities(virConnectPtr conn) { return caps; no_memory: - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + testError(conn, VIR_ERR_NO_MEMORY, NULL); virCapabilitiesFree(caps); return NULL; } @@ -209,13 +207,13 @@ static int testOpenDefault(virConnectPtr conn) { virNetworkObjPtr netobj = NULL; if (VIR_ALLOC(privconn) < 0) { - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, "testConn"); + testError(conn, VIR_ERR_NO_MEMORY, "testConn"); return VIR_DRV_OPEN_ERROR; } conn->privateData = privconn; if (gettimeofday(&tv, NULL) < 0) { - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("getting time of day")); + testError(NULL, VIR_ERR_INTERNAL_ERROR, _("getting time of day")); goto error; } @@ -304,7 +302,7 @@ static int testOpenFromFile(virConnectPtr conn, virDomainObjPtr dom; testConnPtr privconn; if (VIR_ALLOC(privconn) < 0) { - testError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, "testConn"); + testError(NULL, VIR_ERR_NO_MEMORY, "testConn"); return VIR_DRV_OPEN_ERROR; } conn->privateData = privconn; @@ -313,14 +311,14 @@ static int testOpenFromFile(virConnectPtr conn, goto error; if ((fd = open(file, O_RDONLY)) < 0) { - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("loading host definition file")); + testError(NULL, VIR_ERR_INTERNAL_ERROR, _("loading host definition file")); goto error; } if (!(xml = xmlReadFd(fd, file, NULL, XML_PARSE_NOENT | XML_PARSE_NONET | XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) { - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("host")); + testError(NULL, VIR_ERR_INTERNAL_ERROR, _("host")); goto error; } close(fd); @@ -328,13 +326,13 @@ static int testOpenFromFile(virConnectPtr conn, root = xmlDocGetRootElement(xml); if ((root == NULL) || (!xmlStrEqual(root->name, BAD_CAST "node"))) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node")); + testError(NULL, VIR_ERR_XML_ERROR, _("node")); goto error; } ctxt = xmlXPathNewContext(xml); if (ctxt == NULL) { - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("creating xpath context")); + testError(NULL, VIR_ERR_INTERNAL_ERROR, _("creating xpath context")); goto error; } @@ -349,7 +347,7 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->nodes = l; } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node cpu numa nodes")); + testError(NULL, VIR_ERR_XML_ERROR, _("node cpu numa nodes")); goto error; } @@ -357,7 +355,7 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->sockets = l; } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node cpu sockets")); + testError(NULL, VIR_ERR_XML_ERROR, _("node cpu sockets")); goto error; } @@ -365,7 +363,7 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->cores = l; } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node cpu cores")); + testError(NULL, VIR_ERR_XML_ERROR, _("node cpu cores")); goto error; } @@ -373,7 +371,7 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->threads = l; } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node cpu threads")); + testError(NULL, VIR_ERR_XML_ERROR, _("node cpu threads")); goto error; } @@ -384,14 +382,14 @@ static int testOpenFromFile(virConnectPtr conn, nodeInfo->cpus = l; } } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node active cpu")); + testError(NULL, VIR_ERR_XML_ERROR, _("node active cpu")); goto error; } ret = virXPathLong(conn, "string(/node/cpu/mhz[1])", ctxt, &l); if (ret == 0) { nodeInfo->mhz = l; } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node cpu mhz")); + testError(NULL, VIR_ERR_XML_ERROR, _("node cpu mhz")); goto error; } @@ -406,13 +404,13 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->memory = l; } else if (ret == -2) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node memory")); + testError(NULL, VIR_ERR_XML_ERROR, _("node memory")); goto error; } ret = virXPathNodeSet(conn, "/node/domain", ctxt, &domains); if (ret < 0) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node domain list")); + testError(NULL, VIR_ERR_XML_ERROR, _("node domain list")); goto error; } @@ -423,7 +421,7 @@ static int testOpenFromFile(virConnectPtr conn, char *absFile = testBuildFilename(file, relFile); VIR_FREE(relFile); if (!absFile) { - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("resolving domain filename")); + testError(NULL, VIR_ERR_INTERNAL_ERROR, _("resolving domain filename")); goto error; } def = virDomainDefParseFile(conn, privconn->caps, absFile); @@ -449,7 +447,7 @@ static int testOpenFromFile(virConnectPtr conn, ret = virXPathNodeSet(conn, "/node/network", ctxt, &networks); if (ret < 0) { - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node network list")); + testError(NULL, VIR_ERR_XML_ERROR, _("node network list")); goto error; } for (i = 0 ; i < ret ; i++) { @@ -459,7 +457,7 @@ static int testOpenFromFile(virConnectPtr conn, char *absFile = testBuildFilename(file, relFile); VIR_FREE(relFile); if (!absFile) { - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("resolving network filename")); + testError(NULL, VIR_ERR_INTERNAL_ERROR, _("resolving network filename")); goto error; } @@ -536,7 +534,7 @@ static int testOpen(virConnectPtr conn, if (!uri->path || uri->path[0] == '\0' || (uri->path[0] == '/' && uri->path[1] == '\0')) { - testError (NULL, NULL, NULL, VIR_ERR_INVALID_ARG, + testError (NULL, VIR_ERR_INVALID_ARG, _("testOpen: supply a path or use test:///default")); return VIR_DRV_OPEN_ERROR; } @@ -587,13 +585,13 @@ static char *testGetHostname (virConnectPtr conn) r = gethostname (hostname, HOST_NAME_MAX+1); if (r == -1) { - testError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, "%s", + testError (conn, VIR_ERR_SYSTEM_ERROR, "%s", strerror (errno)); return NULL; } str = strdup (hostname); if (str == NULL) { - testError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, "%s", + testError (conn, VIR_ERR_SYSTEM_ERROR, "%s", strerror (errno)); return NULL; } @@ -606,7 +604,7 @@ static char * testGetURI (virConnectPtr conn) GET_CONNECTION(conn); if (asprintf (&uri, "test://%s", privconn->path) == -1) { - testError (conn, NULL, NULL, VIR_ERR_SYSTEM_ERROR, "%s", + testError (conn, VIR_ERR_SYSTEM_ERROR, "%s", strerror (errno)); return NULL; } @@ -633,7 +631,7 @@ static char *testGetCapabilities (virConnectPtr conn) GET_CONNECTION(conn); if ((xml = virCapabilitiesFormatXML(privconn->caps)) == NULL) { - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + testError(conn, VIR_ERR_NO_MEMORY, NULL); return NULL; } @@ -691,7 +689,7 @@ static virDomainPtr testLookupDomainByID(virConnectPtr conn, GET_CONNECTION(conn); if ((dom = virDomainFindByID(privconn->domains, id)) == NULL) { - testError (conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL); + testError (conn, VIR_ERR_NO_DOMAIN, NULL); return NULL; } @@ -710,7 +708,7 @@ static virDomainPtr testLookupDomainByUUID(virConnectPtr conn, GET_CONNECTION(conn); if ((dom = virDomainFindByUUID(privconn->domains, uuid)) == NULL) { - testError (conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL); + testError (conn, VIR_ERR_NO_DOMAIN, NULL); return NULL; } @@ -729,7 +727,7 @@ static virDomainPtr testLookupDomainByName(virConnectPtr conn, GET_CONNECTION(conn); if ((dom = virDomainFindByName(privconn->domains, name)) == NULL) { - testError (conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL); + testError (conn, VIR_ERR_NO_DOMAIN, NULL); return NULL; } @@ -776,7 +774,7 @@ static int testResumeDomain (virDomainPtr domain) GET_DOMAIN(domain, -1); if (privdom->state != VIR_DOMAIN_PAUSED) { - testError(domain->conn, NULL, NULL, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: domain not paused"), domain->name); return -1; @@ -792,7 +790,7 @@ static int testPauseDomain (virDomainPtr domain) if (privdom->state == VIR_DOMAIN_SHUTOFF || privdom->state == VIR_DOMAIN_PAUSED) { - testError(domain->conn, NULL, NULL, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: domain not running"), domain->name); return -1; @@ -807,7 +805,7 @@ static int testShutdownDomain (virDomainPtr domain) GET_DOMAIN(domain, -1); if (privdom->state == VIR_DOMAIN_SHUTOFF) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: domain not running"), domain->name); return -1; } @@ -864,7 +862,7 @@ static int testGetDomainInfo (virDomainPtr domain, GET_DOMAIN(domain, -1); if (gettimeofday(&tv, NULL) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("getting time of day")); return (-1); } @@ -890,31 +888,31 @@ static int testDomainSave(virDomainPtr domain, xml = testDomainDumpXML(domain, 0); if (xml == NULL) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: cannot allocate space for metadata"), domain->name); return (-1); } if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: cannot save domain"), domain->name); return (-1); } len = strlen(xml); if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: cannot write header"), domain->name); close(fd); return (-1); } if (safewrite(fd, (char*)&len, sizeof(len)) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: cannot write metadata length"), domain->name); close(fd); return (-1); } if (safewrite(fd, xml, len) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: cannot write metadata"), domain->name); VIR_FREE(xml); close(fd); @@ -922,7 +920,7 @@ static int testDomainSave(virDomainPtr domain, } VIR_FREE(xml); if (close(fd) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("%s: cannot save domain data"), domain->name); close(fd); return (-1); @@ -946,41 +944,41 @@ static int testDomainRestore(virConnectPtr conn, GET_CONNECTION(conn); if ((fd = open(path, O_RDONLY)) < 0) { - testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot read domain image")); return (-1); } if (read(fd, magic, sizeof(magic)) != sizeof(magic)) { - testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(conn, VIR_ERR_INTERNAL_ERROR, _("incomplete save header")); close(fd); return (-1); } if (memcmp(magic, TEST_SAVE_MAGIC, sizeof(magic))) { - testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(conn, VIR_ERR_INTERNAL_ERROR, _("mismatched header magic")); close(fd); return (-1); } if (read(fd, (char*)&len, sizeof(len)) != sizeof(len)) { - testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(conn, VIR_ERR_INTERNAL_ERROR, _("failed to read metadata length")); close(fd); return (-1); } if (len < 1 || len > 8192) { - testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(conn, VIR_ERR_INTERNAL_ERROR, _("length of metadata out of range")); close(fd); return (-1); } if (VIR_ALLOC_N(xml, len+1) < 0) { - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, "xml"); + testError(conn, VIR_ERR_NO_MEMORY, "xml"); close(fd); return (-1); } if (read(fd, xml, len) != len) { - testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(conn, VIR_ERR_INTERNAL_ERROR, _("incomplete metdata")); close(fd); return (-1); @@ -1011,20 +1009,20 @@ static int testDomainCoreDump(virDomainPtr domain, GET_DOMAIN(domain, -1); if ((fd = open(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("domain '%s' coredump: failed to open %s: %s"), domain->name, to, strerror (errno)); return (-1); } if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("domain '%s' coredump: failed to write header to %s: %s"), domain->name, to, strerror (errno)); close(fd); return (-1); } if (close(fd) < 0) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("domain '%s' coredump: write failed: %s: %s"), domain->name, to, strerror (errno)); close(fd); @@ -1041,7 +1039,7 @@ static int testDomainCoreDump(virDomainPtr domain, static char *testGetOSType(virDomainPtr dom) { char *ret = strdup("linux"); if (!ret) - testError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY, NULL); + testError(dom->conn, VIR_ERR_NO_MEMORY, NULL); return ret; } @@ -1067,7 +1065,7 @@ static int testSetMemory(virDomainPtr domain, GET_DOMAIN(domain, -1); if (memory > privdom->def->maxmem) { - testError(domain->conn, NULL, NULL, + testError(domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); } @@ -1082,7 +1080,7 @@ static int testSetVcpus(virDomainPtr domain, /* We allow more cpus in guest than host */ if (nrCpus > 32) { - testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); + testError(domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); } @@ -1135,7 +1133,7 @@ static int testListDefinedDomains(virConnectPtr conn, return n; no_memory: - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + testError(conn, VIR_ERR_NO_MEMORY, NULL); for (n = 0 ; n < maxnames ; n++) VIR_FREE(names[n]); return -1; @@ -1174,7 +1172,7 @@ static int testNodeGetCellsFreeMemory(virConnectPtr conn, GET_CONNECTION(conn); if (startCell > privconn->numCells) { - testError(conn, NULL, NULL, VIR_ERR_INVALID_ARG, + testError(conn, VIR_ERR_INVALID_ARG, _("Range exceeds available cells")); return -1; } @@ -1193,7 +1191,7 @@ static int testDomainCreate(virDomainPtr domain) { GET_DOMAIN(domain, -1); if (privdom->state != VIR_DOMAIN_SHUTOFF) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("Domain '%s' is already running"), domain->name); return (-1); } @@ -1208,7 +1206,7 @@ static int testDomainUndefine(virDomainPtr domain) { GET_DOMAIN(domain, -1); if (privdom->state != VIR_DOMAIN_SHUTOFF) { - testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(domain->conn, VIR_ERR_INTERNAL_ERROR, _("Domain '%s' is still running"), domain->name); return (-1); } @@ -1244,7 +1242,7 @@ static char *testDomainGetSchedulerType(virDomainPtr domain, *nparams = 1; type = strdup("fair"); if (!type) { - testError(domain->conn, NULL, NULL, VIR_ERR_NO_MEMORY, "schedular"); + testError(domain->conn, VIR_ERR_NO_MEMORY, "schedular"); return (NULL); } return type; @@ -1256,7 +1254,7 @@ static int testDomainGetSchedulerParams(virDomainPtr domain, { GET_DOMAIN(domain, -1); if (*nparams != 1) { - testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "nparams"); + testError(domain->conn, VIR_ERR_INVALID_ARG, "nparams"); return (-1); } strcpy(params[0].field, "weight"); @@ -1274,15 +1272,15 @@ static int testDomainSetSchedulerParams(virDomainPtr domain, { GET_DOMAIN(domain, -1); if (nparams != 1) { - testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "nparams"); + testError(domain->conn, VIR_ERR_INVALID_ARG, "nparams"); return (-1); } if (STRNEQ(params[0].field, "weight")) { - testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "field"); + testError(domain->conn, VIR_ERR_INVALID_ARG, "field"); return (-1); } if (params[0].type != VIR_DOMAIN_SCHED_FIELD_UINT) { - testError(domain->conn, NULL, NULL, VIR_ERR_INVALID_ARG, "type"); + testError(domain->conn, VIR_ERR_INVALID_ARG, "type"); return (-1); } /* XXX */ @@ -1314,7 +1312,7 @@ static virNetworkPtr testLookupNetworkByUUID(virConnectPtr conn, GET_CONNECTION(conn); if ((net = virNetworkFindByUUID(privconn->networks, uuid)) == NULL) { - testError (conn, NULL, NULL, VIR_ERR_NO_NETWORK, NULL); + testError (conn, VIR_ERR_NO_NETWORK, NULL); return NULL; } @@ -1328,7 +1326,7 @@ static virNetworkPtr testLookupNetworkByName(virConnectPtr conn, GET_CONNECTION(conn); if ((net = virNetworkFindByName(privconn->networks, name)) == NULL) { - testError (conn, NULL, NULL, VIR_ERR_NO_NETWORK, NULL); + testError (conn, VIR_ERR_NO_NETWORK, NULL); return NULL; } @@ -1366,7 +1364,7 @@ static int testListNetworks(virConnectPtr conn, char **const names, int nnames) return n; no_memory: - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + testError(conn, VIR_ERR_NO_MEMORY, NULL); for (n = 0 ; n < nnames ; n++) VIR_FREE(names[n]); return (-1); @@ -1402,7 +1400,7 @@ static int testListDefinedNetworks(virConnectPtr conn, char **const names, int n return n; no_memory: - testError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + testError(conn, VIR_ERR_NO_MEMORY, NULL); for (n = 0 ; n < nnames ; n++) VIR_FREE(names[n]); return (-1); @@ -1448,7 +1446,7 @@ static int testNetworkUndefine(virNetworkPtr network) { GET_NETWORK(network, -1); if (virNetworkIsActive(privnet)) { - testError(network->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(network->conn, VIR_ERR_INTERNAL_ERROR, _("Network '%s' is still running"), network->name); return (-1); } @@ -1463,7 +1461,7 @@ static int testNetworkStart(virNetworkPtr network) { GET_NETWORK(network, -1); if (virNetworkIsActive(privnet)) { - testError(network->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + testError(network->conn, VIR_ERR_INTERNAL_ERROR, _("Network '%s' is already running"), network->name); return (-1); } @@ -1495,7 +1493,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { GET_NETWORK(network, NULL); if (privnet->def->bridge && !(bridge = strdup(privnet->def->bridge))) { - testError(network->conn, NULL, NULL, VIR_ERR_NO_MEMORY, "network"); + testError(network->conn, VIR_ERR_NO_MEMORY, "network"); return NULL; } return bridge; -- 1.6.0.2.307.gc4275
On Fri, Oct 10, 2008 at 03:16:09PM +0200, Jim Meyering wrote: > Jim Meyering <jim@meyering.net> wrote: > > > "Daniel P. Berrange" <berrange@redhat.com> wrote: > Here's the change to adjust src/test.c. > Two change sets: > - Fix the "make check" failure by adjusting uses of testError, > including domain->name (and a few net->name) strings via format. > - Remove now-ignored dom and net parameters. Sounds fine to me. I saw 2 concatenated patches (mutt here), I didn't spot any problem, +1 thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Oct 10, 2008 at 03:16:09PM +0200, Jim Meyering wrote:
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Some parts of qemu supply it, many other parts do not since they have no access to the virDomainPtr object. This is one of the reasons for deprecating this field - it was impossible to reliably provide it when raising errors.
In this particular test case error, we are better off supplying the domain name in the format string - it'll improve the error message to have it placed in context of the description
eg, instead of
libvir: Test error test: internal error Domain is still running
We'd have
libvir: Test error: internal error Domain 'test' is still running
Which could be done by changing
if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain is still running"));
to be
if (privdom->state != VIR_DOMAIN_SHUTOFF) { testError(domain->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("Domain '%s' is still running"), domain->name);
Yep. I began doing that about 15 minutes ago ;-)
Here's the change to adjust src/test.c. Two change sets: - Fix the "make check" failure by adjusting uses of testError, including domain->name (and a few net->name) strings via format. - Remove now-ignored dom and net parameters.
ACK, these both look good. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering