In one of my previous commits, I've changed an XPath in
virCPUDefParseXML() from "boolean(./counter...)" to
"./counter...)". Notice the dangling closing bracket? Well, I
didn't back then.

Fixes: 0fe2d8dd335054fae38b46bbbac58a4662e1a1d0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/cpu_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index da83e99371..819efcea8c 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -441,7 +441,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
             return -1;
         }
 
-        if ((counter_node = virXPathNode("./counter[@name='tsc'])", ctxt))) {
+        if ((counter_node = virXPathNode("./counter[@name='tsc']", ctxt))) {
             tsc = g_new0(virHostCPUTscInfo, 1);
 
             if (virXMLPropULongLong(counter_node, "frequency", 10,
-- 
2.34.1

On a Tuesday in 2022, Michal Privoznik wrote:
>In one of my previous commits, I've changed an XPath in
>virCPUDefParseXML() from "boolean(./counter...)" to
>"./counter...)". Notice the dangling closing bracket? Well, I
>didn't back then.
>
>Fixes: 0fe2d8dd335054fae38b46bbbac58a4662e1a1d0
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/conf/cpu_conf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

On Tue, Feb 08, 2022 at 12:22:36PM +0100, Michal Privoznik wrote:
> In one of my previous commits, I've changed an XPath in
> virCPUDefParseXML() from "boolean(./counter...)" to
> "./counter...)". Notice the dangling closing bracket? Well, I
> didn't back then.

Suggests we have missing test XML data file example to
be added somewhere, as detecting parsing errors are the
one thing we are pretty good at in unit tests usually.

> 
> Fixes: 0fe2d8dd335054fae38b46bbbac58a4662e1a1d0
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/conf/cpu_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index da83e99371..819efcea8c 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -441,7 +441,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
>              return -1;
>          }
>  
> -        if ((counter_node = virXPathNode("./counter[@name='tsc'])", ctxt))) {
> +        if ((counter_node = virXPathNode("./counter[@name='tsc']", ctxt))) {
>              tsc = g_new0(virHostCPUTscInfo, 1);
>  
>              if (virXMLPropULongLong(counter_node, "frequency", 10,
> -- 
> 2.34.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 2/9/22 10:39, Daniel P. Berrangé wrote:
> On Tue, Feb 08, 2022 at 12:22:36PM +0100, Michal Privoznik wrote:
>> In one of my previous commits, I've changed an XPath in
>> virCPUDefParseXML() from "boolean(./counter...)" to
>> "./counter...)". Notice the dangling closing bracket? Well, I
>> didn't back then.
> 
> Suggests we have missing test XML data file example to
> be added somewhere, as detecting parsing errors are the
> one thing we are pretty good at in unit tests usually.

Actually we do have a test, well, sort of. We have cputest which if ran
standalone prints errors onto stderr:

ibvirt.git/_build/tests $  ./cputest
TEST: cputest
      XPath error : Invalid expression
./counter[@name='tsc'])
                      ^
But since everybody resorts to plain 'ninja test' which cleverly
discards these errors (storing them in a file that nobody reads is
equivalent) it went undetected. On the other hand, making a test fail on
nonempty stderr feels wrong. With the old suite we at least saw stderr
interleaved with regular output 😞.

Michal

On Wed, Feb 09, 2022 at 14:28:29 +0100, Michal Prívozník wrote:
> On 2/9/22 10:39, Daniel P. Berrangé wrote:
> > On Tue, Feb 08, 2022 at 12:22:36PM +0100, Michal Privoznik wrote:
> >> In one of my previous commits, I've changed an XPath in
> >> virCPUDefParseXML() from "boolean(./counter...)" to
> >> "./counter...)". Notice the dangling closing bracket? Well, I
> >> didn't back then.
> > 
> > Suggests we have missing test XML data file example to
> > be added somewhere, as detecting parsing errors are the
> > one thing we are pretty good at in unit tests usually.
> 
> Actually we do have a test, well, sort of. We have cputest which if ran
> standalone prints errors onto stderr:
> 
> ibvirt.git/_build/tests $  ./cputest
> TEST: cputest
>       XPath error : Invalid expression
> ./counter[@name='tsc'])
>                       ^
> But since everybody resorts to plain 'ninja test' which cleverly
> discards these errors (storing them in a file that nobody reads is
> equivalent) it went undetected. On the other hand, making a test fail on
> nonempty stderr feels wrong. With the old suite we at least saw stderr
> interleaved with regular output 😞.

Apparently the root cause in virCPUDefParseXML which just ignores this
kind of failure. A failure to get a value by XPath is just interpreted
as if the corresponding value was not present in the XML no matter what
exactly failed and why.

Jirka

On Wed, Feb 09, 2022 at 02:28:29PM +0100, Michal Prívozník wrote:
> On 2/9/22 10:39, Daniel P. Berrangé wrote:
> > On Tue, Feb 08, 2022 at 12:22:36PM +0100, Michal Privoznik wrote:
> >> In one of my previous commits, I've changed an XPath in
> >> virCPUDefParseXML() from "boolean(./counter...)" to
> >> "./counter...)". Notice the dangling closing bracket? Well, I
> >> didn't back then.
> > 
> > Suggests we have missing test XML data file example to
> > be added somewhere, as detecting parsing errors are the
> > one thing we are pretty good at in unit tests usually.
> 
> Actually we do have a test, well, sort of. We have cputest which if ran
> standalone prints errors onto stderr:
> 
> ibvirt.git/_build/tests $  ./cputest
> TEST: cputest
>       XPath error : Invalid expression
> ./counter[@name='tsc'])
>                       ^
> But since everybody resorts to plain 'ninja test' which cleverly
> discards these errors (storing them in a file that nobody reads is
> equivalent) it went undetected. On the other hand, making a test fail on
> nonempty stderr feels wrong. With the old suite we at least saw stderr
> interleaved with regular output 😞.

Hmm, so we have two bugs in fact

First our virXPathNode() API is broken by design because it overloads
"NULL" return value to be both a valid condition and an indication
of error. It needs to return an integer 0 / -1, and have the
xmlNodePtr as an output parameter, so that callers can distinguish
the two scenarios.

Second, something (presumably libxml) is printing to stderr which
is undesirable. IIRC libxml shared the same design flaw as libvirt
in printing errors to stderr by default unless you set a global
error reporting func to stop this.  We can't set that func in
libvirt.so code as that effects the whole process. We could set
it in the daemons, or in the tests though.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|