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