5 a.m.
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/0418f90809aea5b375c859e744c...
[2] : https://github.com/aharivel/libvirt/commit/153a4489f095e063f3c7e0470a8f92...
5:49 a.m.
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
5:56 a.m.
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.
}
4:28 a.m.
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
6:36 a.m.
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.
95
days inactive
96
days old
4 comments
2 participants
participants (2)
-
Anthony Harivel -
Peter Krempa