[libvirt] [PATCH] Fix messages using VIR_ERR_XML_ERROR
This error code has existed since the dawn of time, yet the messages it generates are almost universally busted. Here's a small sampling: src/conf/domain_conf.c:4889 : XML description for missing root element is not well formed or invalid src/conf/domain_conf.c:4951 : XML description for unknown device type is not well formed or invalid src/conf/domain_conf.c:5460 : XML description for maximum vcpus must be an integer is not well formed or invalid src/conf/domain_conf.c:5468 : XML description for invalid maxvcpus %(count)lu is not well formed or invalid Fix up the error code to instead be XML error: <msg> Adjust the few locations that we using the original correctly (or shouldn't have been using the error code at all). Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 21 ++++++++++++++------- src/util/virterror.c | 4 ++-- src/xen/xm_internal.c | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 119b027..a9b306e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -811,7 +811,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->nodes = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu numa nodes")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu nodes value")); goto error; } @@ -819,7 +820,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->sockets = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu sockets")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu sockets value")); goto error; } @@ -827,7 +829,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->cores = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu cores")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu cores value")); goto error; } @@ -835,7 +838,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->threads = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu threads")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu threads value")); goto error; } @@ -846,14 +850,16 @@ static int testOpenFromFile(virConnectPtr conn, nodeInfo->cpus = l; } } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node active cpu")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu active value")); goto error; } ret = virXPathLong("string(/node/cpu/mhz[1])", ctxt, &l); if (ret == 0) { nodeInfo->mhz = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu mhz")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu mhz value")); goto error; } @@ -872,7 +878,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->memory = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node memory")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node memory value")); goto error; } diff --git a/src/util/virterror.c b/src/util/virterror.c index fbb4a45..881a7dc 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -925,9 +925,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_XML_ERROR: if (info == NULL) - errmsg = _("XML description not well formed or invalid"); + errmsg = _("XML description is not well formed or invalid"); else - errmsg = _("XML description for %s is not well formed or invalid"); + errmsg = _("XML error: %s"); break; case VIR_ERR_DOM_EXIST: if (info == NULL) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 69e14c3..d0035c9 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1515,8 +1515,9 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, break; } default: - xenXMError(VIR_ERR_XML_ERROR, - "%s", _("unknown device")); + xenXMError(VIR_ERR_CONFIG_UNSUPPORTED, + _("device type '%s' cannot be detached"), + virDomainDeviceTypeToString(dev->type)); goto cleanup; } -- 1.7.4.4
On 05/12/2011 11:47 PM, Cole Robinson wrote:
This error code has existed since the dawn of time, yet the messages it generates are almost universally busted. Here's a small sampling:
src/conf/domain_conf.c:4889 : XML description for missing root element is not well formed or invalid src/conf/domain_conf.c:4951 : XML description for unknown device type is not well formed or invalid src/conf/domain_conf.c:5460 : XML description for maximum vcpus must be an integer is not well formed or invalid src/conf/domain_conf.c:5468 : XML description for invalid maxvcpus %(count)lu is not well formed or invalid
Fix up the error code to instead be
XML error: <msg>
Adjust the few locations that we using the original correctly (or shouldn't have been using the error code at all).
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 21 ++++++++++++++------- src/util/virterror.c | 4 ++-- src/xen/xm_internal.c | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 119b027..a9b306e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -811,7 +811,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->nodes = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu numa nodes")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu nodes value")); goto error; }
@@ -819,7 +820,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->sockets = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu sockets")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu sockets value")); goto error; }
@@ -827,7 +829,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->cores = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu cores")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu cores value")); goto error; }
@@ -835,7 +838,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->threads = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu threads")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu threads value")); goto error; }
@@ -846,14 +850,16 @@ static int testOpenFromFile(virConnectPtr conn, nodeInfo->cpus = l; } } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node active cpu")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu active value")); goto error; } ret = virXPathLong("string(/node/cpu/mhz[1])", ctxt, &l); if (ret == 0) { nodeInfo->mhz = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu mhz")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu mhz value")); goto error; }
@@ -872,7 +878,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->memory = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node memory")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node memory value")); goto error; }
diff --git a/src/util/virterror.c b/src/util/virterror.c index fbb4a45..881a7dc 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -925,9 +925,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_XML_ERROR: if (info == NULL) - errmsg = _("XML description not well formed or invalid"); + errmsg = _("XML description is not well formed or invalid"); else - errmsg = _("XML description for %s is not well formed or invalid"); + errmsg = _("XML error: %s"); break; case VIR_ERR_DOM_EXIST: if (info == NULL) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 69e14c3..d0035c9 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1515,8 +1515,9 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, break; } default: - xenXMError(VIR_ERR_XML_ERROR, - "%s", _("unknown device")); + xenXMError(VIR_ERR_CONFIG_UNSUPPORTED, + _("device type '%s' cannot be detached"), + virDomainDeviceTypeToString(dev->type)); goto cleanup; }
Well, I've started to work on this weeks ago, but left it for more important stuff. I've created new error code, to distinguish XML errors (not well formed XML), bad values (e.g. string given when uint is expected), and unsupported values. But you got a good point. Michal
On 05/13/2011 08:35 AM, Michal Prívozník wrote:
On 05/12/2011 11:47 PM, Cole Robinson wrote:
This error code has existed since the dawn of time, yet the messages it generates are almost universally busted. Here's a small sampling:
src/conf/domain_conf.c:4889 : XML description for missing root element is not well formed or invalid src/conf/domain_conf.c:4951 : XML description for unknown device type is not well formed or invalid src/conf/domain_conf.c:5460 : XML description for maximum vcpus must be an integer is not well formed or invalid src/conf/domain_conf.c:5468 : XML description for invalid maxvcpus %(count)lu is not well formed or invalid
Fix up the error code to instead be
XML error: <msg>
Adjust the few locations that we using the original correctly (or shouldn't have been using the error code at all).
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 21 ++++++++++++++------- src/util/virterror.c | 4 ++-- src/xen/xm_internal.c | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 119b027..a9b306e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -811,7 +811,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->nodes = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu numa nodes")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu nodes value")); goto error; }
@@ -819,7 +820,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->sockets = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu sockets")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu sockets value")); goto error; }
@@ -827,7 +829,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->cores = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu cores")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu cores value")); goto error; }
@@ -835,7 +838,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->threads = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu threads")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu threads value")); goto error; }
@@ -846,14 +850,16 @@ static int testOpenFromFile(virConnectPtr conn, nodeInfo->cpus = l; } } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node active cpu")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu active value")); goto error; } ret = virXPathLong("string(/node/cpu/mhz[1])", ctxt, &l); if (ret == 0) { nodeInfo->mhz = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu mhz")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu mhz value")); goto error; }
@@ -872,7 +878,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->memory = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node memory")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node memory value")); goto error; }
diff --git a/src/util/virterror.c b/src/util/virterror.c index fbb4a45..881a7dc 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -925,9 +925,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_XML_ERROR: if (info == NULL) - errmsg = _("XML description not well formed or invalid"); + errmsg = _("XML description is not well formed or invalid"); else - errmsg = _("XML description for %s is not well formed or invalid"); + errmsg = _("XML error: %s"); break; case VIR_ERR_DOM_EXIST: if (info == NULL) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 69e14c3..d0035c9 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1515,8 +1515,9 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, break; } default: - xenXMError(VIR_ERR_XML_ERROR, - "%s", _("unknown device")); + xenXMError(VIR_ERR_CONFIG_UNSUPPORTED, + _("device type '%s' cannot be detached"), + virDomainDeviceTypeToString(dev->type)); goto cleanup; }
Well, I've started to work on this weeks ago, but left it for more important stuff. I've created new error code, to distinguish XML errors (not well formed XML), bad values (e.g. string given when uint is expected), and unsupported values. But you got a good point.
That certainly sounds useful. Any objection to this patch in the mean time though? Thanks, Cole
On 18.05.2011 15:41, Cole Robinson wrote:
On 05/13/2011 08:35 AM, Michal Prívozník wrote:
On 05/12/2011 11:47 PM, Cole Robinson wrote:
This error code has existed since the dawn of time, yet the messages it generates are almost universally busted. Here's a small sampling:
src/conf/domain_conf.c:4889 : XML description for missing root element is not well formed or invalid src/conf/domain_conf.c:4951 : XML description for unknown device type is not well formed or invalid src/conf/domain_conf.c:5460 : XML description for maximum vcpus must be an integer is not well formed or invalid src/conf/domain_conf.c:5468 : XML description for invalid maxvcpus %(count)lu is not well formed or invalid
Fix up the error code to instead be
XML error: <msg>
Adjust the few locations that we using the original correctly (or shouldn't have been using the error code at all).
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 21 ++++++++++++++------- src/util/virterror.c | 4 ++-- src/xen/xm_internal.c | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 119b027..a9b306e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -811,7 +811,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->nodes = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu numa nodes")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu nodes value")); goto error; }
@@ -819,7 +820,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->sockets = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu sockets")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu sockets value")); goto error; }
@@ -827,7 +829,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->cores = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu cores")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu cores value")); goto error; }
@@ -835,7 +838,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->threads = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu threads")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu threads value")); goto error; }
@@ -846,14 +850,16 @@ static int testOpenFromFile(virConnectPtr conn, nodeInfo->cpus = l; } } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node active cpu")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu active value")); goto error; } ret = virXPathLong("string(/node/cpu/mhz[1])", ctxt, &l); if (ret == 0) { nodeInfo->mhz = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node cpu mhz")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node cpu mhz value")); goto error; }
@@ -872,7 +878,8 @@ static int testOpenFromFile(virConnectPtr conn, if (ret == 0) { nodeInfo->memory = l; } else if (ret == -2) { - testError(VIR_ERR_XML_ERROR, "%s", _("node memory")); + testError(VIR_ERR_XML_ERROR, "%s", + _("invalid node memory value")); goto error; }
diff --git a/src/util/virterror.c b/src/util/virterror.c index fbb4a45..881a7dc 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -925,9 +925,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_XML_ERROR: if (info == NULL) - errmsg = _("XML description not well formed or invalid"); + errmsg = _("XML description is not well formed or invalid"); else - errmsg = _("XML description for %s is not well formed or invalid"); + errmsg = _("XML error: %s"); break; case VIR_ERR_DOM_EXIST: if (info == NULL) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 69e14c3..d0035c9 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1515,8 +1515,9 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, break; } default: - xenXMError(VIR_ERR_XML_ERROR, - "%s", _("unknown device")); + xenXMError(VIR_ERR_CONFIG_UNSUPPORTED, + _("device type '%s' cannot be detached"), + virDomainDeviceTypeToString(dev->type)); goto cleanup; }
Well, I've started to work on this weeks ago, but left it for more important stuff. I've created new error code, to distinguish XML errors (not well formed XML), bad values (e.g. string given when uint is expected), and unsupported values. But you got a good point.
That certainly sounds useful. Any objection to this patch in the mean time though?
Thanks, Cole
Certainly not. The places you fix now produce really weird error messages, as you mentioned above. ACK Michal
On 05/12/2011 03:47 PM, Cole Robinson wrote:
This error code has existed since the dawn of time, yet the messages it generates are almost universally busted. Here's a small sampling:
src/conf/domain_conf.c:4889 : XML description for missing root element is not well formed or invalid src/conf/domain_conf.c:4951 : XML description for unknown device type is not well formed or invalid src/conf/domain_conf.c:5460 : XML description for maximum vcpus must be an integer is not well formed or invalid src/conf/domain_conf.c:5468 : XML description for invalid maxvcpus %(count)lu is not well formed or invalid
Fix up the error code to instead be
XML error: <msg>
Adjust the few locations that we using the original correctly (or shouldn't
s/we/were/
have been using the error code at all).
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 21 ++++++++++++++------- src/util/virterror.c | 4 ++-- src/xen/xm_internal.c | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-)
ACK; even if we further improve things later, this patch is a definite improvement on its own merits.
+++ b/src/util/virterror.c @@ -925,9 +925,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_XML_ERROR: if (info == NULL) - errmsg = _("XML description not well formed or invalid"); + errmsg = _("XML description is not well formed or invalid");
The placement of "not" makes this a bit ambiguous; is it worth using: "XML description is invalid or not well formed" -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/18/2011 10:03 AM, Eric Blake wrote:
On 05/12/2011 03:47 PM, Cole Robinson wrote:
This error code has existed since the dawn of time, yet the messages it generates are almost universally busted. Here's a small sampling:
src/conf/domain_conf.c:4889 : XML description for missing root element is not well formed or invalid src/conf/domain_conf.c:4951 : XML description for unknown device type is not well formed or invalid src/conf/domain_conf.c:5460 : XML description for maximum vcpus must be an integer is not well formed or invalid src/conf/domain_conf.c:5468 : XML description for invalid maxvcpus %(count)lu is not well formed or invalid
Fix up the error code to instead be
XML error: <msg>
Adjust the few locations that we using the original correctly (or shouldn't
s/we/were/
have been using the error code at all).
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 21 ++++++++++++++------- src/util/virterror.c | 4 ++-- src/xen/xm_internal.c | 5 +++-- 3 files changed, 19 insertions(+), 11 deletions(-)
ACK; even if we further improve things later, this patch is a definite improvement on its own merits.
+++ b/src/util/virterror.c @@ -925,9 +925,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_XML_ERROR: if (info == NULL) - errmsg = _("XML description not well formed or invalid"); + errmsg = _("XML description is not well formed or invalid");
The placement of "not" makes this a bit ambiguous; is it worth using:
"XML description is invalid or not well formed"
Thanks, pushed with those 2 recommended tweaks. - Cole
participants (3)
-
Cole Robinson -
Eric Blake -
Michal Prívozník