[libvirt] [PATCH] xml: Make sure virXpathNodeSet always sets an error

[Cole Robinson]

On 05/12/2011 07:03 PM, Eric Blake wrote:
> On 05/12/2011 03:47 PM, Cole Robinson wrote:
>> And update callers to actually respect the error
>>
>> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>> ---
>>  src/conf/domain_conf.c             |   38 ------------------------------------
>>  src/conf/interface_conf.c          |   15 ++++++++++---
>>  src/conf/network_conf.c            |    3 ++
>>  src/conf/node_device_conf.c        |   12 ++++------
>>  src/conf/storage_conf.c            |    3 ++
>>  src/conf/storage_encryption_conf.c |    2 -
>>  src/qemu/qemu_domain.c             |    2 -
>>  src/test/test_driver.c             |    7 ------
>>  src/util/xml.c                     |    7 +++++-
>>  9 files changed, 28 insertions(+), 61 deletions(-)
>>
> 
>> +++ b/src/util/xml.c
>> @@ -589,7 +589,7 @@ virXPathNodeSet(const char *xpath,
>>  
>>      if ((ctxt == NULL) || (xpath == NULL)) {
>>          virXMLError(VIR_ERR_INTERNAL_ERROR,
>> -                    "%s", _("Invalid parameter to virXPathNodeSet()"));
>> +                    _("Invalid parameter to %s"), __FUNCTION__);
> 
> Pre-existing, but virXMLError already outputs __FUNCTION__ earlier in
> the line.

Hmm, does it? It passed __FUNCTION__ to virReportErrorHelper, but
doesn't seem to explicitly include it in the error.

 Any error message that lists its own name (whether explicitly
> as in pre-patch or implicitly with __FUNCTION__ or __func__ as in
> post-patch) looks silly.
> 

In general I agree, but I think there is an argument for including it in
these functions, since most of the non-OOM errors are likely the result
of misusing the internal API, so being able to easily identify the
culprit can help. Otherwise this error is simply 'Invalid parameter'.

>>  
>> @@ -601,10 +601,15 @@ virXPathNodeSet(const char *xpath,
>>      ctxt->node = relnode;
>>      if (obj == NULL)
>>          return(0);
>> +
>>      if (obj->type != XPATH_NODESET) {
>> +        virXMLError(VIR_ERR_INTERNAL_ERROR,
>> +                    _("Incorrect xpath '%s' passed to %s"),
>> +                    xpath, __FUNCTION__);
> 
> Likewise, and newly introduced.  Here, you could get by with:
> _("Incorrect xpath '%s'"), xpath
> 
> However, since the problem with __FUNCTION__ is more widespread than
> your patch, I'm okay whether you squash in a change to tweak those
> messages or save it for a a separate cleanup.
> 

I've dropped that first hunk and made the change you recommend here.
Pushed now.

Thanks,
Cole