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(a)redhat.com&gt; --- 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; }

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(a)redhat.com&gt; >> --- >> 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 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(a)redhat.com&gt; > --- > 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"