[libvirt] [PATCH] Fix messages using VIR_ERR_XML_ERROR

[Michal Prívozník]

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 at 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