New features tests fails
Hi, I'm trying to implement the libvirt part of a new feature that has landed in QEMU staging [1]. The current draft of this implementations is the following [2]. It compiles on top of master. But when I want to run the tests suites, I hit an issue I don't managed to find the problem. It runs until: [...] 278/279 libvirt:bin / qemuxmlconftest FAIL 8.63s exit status 1 [...] then it tells me to run the following to understand the issue: VIR_TEST_DEBUG=1 VIR_TEST_RANGE=850-851,854-855 /home/aharivel/work/libvirt/build/tests/qemuxmlconftest It outputs this: [...] 850) QEMU XML def -> XML kvm-features.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features.x86_64-latest.xml': Offset 533 Expect [ socket='/run/qemu-vmsr-helper.sock'/] Actual [/] ... FAILED 851) QEMU XML OUT -> XML kvm-features.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features.x86_64-latest.xml': Offset 533 Expect [ socket='/run/qemu-vmsr-helper.sock'/] Actual [/] ... FAILED 854) QEMU XML def -> XML kvm-features-off.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features-off.x86_64-latest.xml': Offset 527 Expect [/] Actual [ socket='(null)'/] ... FAILED 855) QEMU XML OUT -> XML kvm-features-off.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features-off.x86_64-latest.xml': Offset 527 Expect [/] Actual [ socket='(null)'/] [...] ... FAILED I certainly did something wrong in the implementation but I don't figure it out. I understand the Expect / Actual tests. I'm guessing the issue is that it needs to run the QEMU version where the feature is possible but I don't see how can tell the test to run this particular version. Any advise would be greatly appreciate. Thanks Regards, Anthony [1] : https://gitlab.com/qemu-project/qemu/-/commit/0418f90809aea5b375c859e744c8e8... [2] : https://github.com/aharivel/libvirt/commit/153a4489f095e063f3c7e0470a8f925a0...
On Mon, Aug 19, 2024 at 12:00:56 +0200, Anthony Harivel wrote:
Hi,
I'm trying to implement the libvirt part of a new feature that has landed in QEMU staging [1]. The current draft of this implementations is the following [2]. It compiles on top of master. But when I want to run the tests suites, I hit an issue I don't managed to find the problem.
It runs until: [...] 278/279 libvirt:bin / qemuxmlconftest FAIL 8.63s exit status 1 [...]
then it tells me to run the following to understand the issue: VIR_TEST_DEBUG=1 VIR_TEST_RANGE=850-851,854-855 /home/aharivel/work/libvirt/build/tests/qemuxmlconftest
It outputs this: [...] 850) QEMU XML def -> XML kvm-features.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features.x86_64-latest.xml': Offset 533 Expect [ socket='/run/qemu-vmsr-helper.sock'/] Actual [/]
These test cases parse an XML and format it back. You've got a bug in the code formatting the socket into the XML: if (def->kvm_features->socket == NULL) { virBufferAsprintf(&childBuf, " socket='%s'/>\n", def->kvm_features->socket); } else { virBufferAddLit(&childBuf, "/>\n"); } The above hunk formats the socket only if it's NULL, and skips it if it's non-null (inverted condition).
851) QEMU XML OUT -> XML kvm-features.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features.x86_64-latest.xml': Offset 533 Expect [ socket='/run/qemu-vmsr-helper.sock'/] Actual [/]
So it's missing in the output file when you provide it in the input file.
... FAILED 854) QEMU XML def -> XML kvm-features-off.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features-off.x86_64-latest.xml': Offset 527 Expect [/] Actual [ socket='(null)'/]
And formats it when it's NULL.
... FAILED 855) QEMU XML OUT -> XML kvm-features-off.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features-off.x86_64-latest.xml': Offset 527 Expect [/] Actual [ socket='(null)'/] [...] ... FAILED
On Mon, Aug 19, 2024 at 12:49:19 +0200, Peter Krempa wrote:
On Mon, Aug 19, 2024 at 12:00:56 +0200, Anthony Harivel wrote:
Hi,
I'm trying to implement the libvirt part of a new feature that has landed in QEMU staging [1]. The current draft of this implementations is the following [2]. It compiles on top of master. But when I want to run the tests suites, I hit an issue I don't managed to find the problem.
It runs until: [...] 278/279 libvirt:bin / qemuxmlconftest FAIL 8.63s exit status 1 [...]
then it tells me to run the following to understand the issue: VIR_TEST_DEBUG=1 VIR_TEST_RANGE=850-851,854-855 /home/aharivel/work/libvirt/build/tests/qemuxmlconftest
It outputs this: [...] 850) QEMU XML def -> XML kvm-features.x86_64-latest ... In '/home/aharivel/work/libvirt/tests/qemuxmlconfdata/kvm-features.x86_64-latest.xml': Offset 533 Expect [ socket='/run/qemu-vmsr-helper.sock'/] Actual [/]
These test cases parse an XML and format it back.
You've got a bug in the code formatting the socket into the XML:
if (def->kvm_features->socket == NULL) { virBufferAsprintf(&childBuf, " socket='%s'/>\n", def->kvm_features->socket);
Also note that for user-provided strings you must use virBufferEscapeString in order to properly escape XML entities within strings. virBufferEscapeString will avoid formatting the whole format string completely when the string argument is NULL. Thus if you just skip the '/>\n' part
} else { virBufferAddLit(&childBuf, "/>\n");
and do this unconditionally you can avoid the condition altogether.
}
Hi Peter,
These test cases parse an XML and format it back.
You've got a bug in the code formatting the socket into the XML:
if (def->kvm_features->socket == NULL) { virBufferAsprintf(&childBuf, " socket='%s'/>\n", def->kvm_features->socket); } else { virBufferAddLit(&childBuf, "/>\n"); }
The above hunk formats the socket only if it's NULL, and skips it if it's non-null (inverted condition).
Doh! I think it's time for me to take some PTO... Thank you ! However, I tried my best to follow your advice using virBufferEscapeString() so that i can get rid of the condition on the socket but it fails for "feature-off". But this is passing the test suite: if (def->kvm_features->features[j] != VIR_TRISTATE_SWITCH_ABSENT) { virBufferAsprintf(&childBuf, "<%s state='%s'", virDomainKVMTypeToString(j), virTristateSwitchTypeToString(def->kvm_features->features[j])); if (def->kvm_features->socket != NULL) { virBufferEscapeString(&childBuf, " socket='%s'/>\n", def->kvm_features->socket); } else { virBufferAddLit(&childBuf, "/>\n"); } } Is ok if I send my patch like this or would you prefer me to dig more into simplifying the code ? Regards, Anthony
On Tue, Aug 20, 2024 at 11:28:05 +0200, Anthony Harivel wrote:
Hi Peter,
These test cases parse an XML and format it back.
You've got a bug in the code formatting the socket into the XML:
if (def->kvm_features->socket == NULL) { virBufferAsprintf(&childBuf, " socket='%s'/>\n", def->kvm_features->socket); } else { virBufferAddLit(&childBuf, "/>\n"); }
The above hunk formats the socket only if it's NULL, and skips it if it's non-null (inverted condition).
Doh! I think it's time for me to take some PTO...
Thank you !
However, I tried my best to follow your advice using virBufferEscapeString() so that i can get rid of the condition on the socket but it fails for "feature-off".
But this is passing the test suite:
I meant like this: if (def->kvm_features->features[j] != VIR_TRISTATE_SWITCH_ABSENT) { virBufferAsprintf(&childBuf, "<%s state='%s'", virDomainKVMTypeToString(j), virTristateSwitchTypeToString(def->kvm_features->features[j])); virBufferEscapeString(&childBuf, " socket='%s'", def->kvm_features->socket); virBufferAddLit(&childBuf, "/>\n"); } (note also the fixed alinment) Note that the parser should also disallow the socket path if the feature is explicitly off.
participants (2)
-
Anthony Harivel -
Peter Krempa