Re: [PATCH qemu-server 2/6] tests: hotplug: add some test cases

[Dominik Csapak <d.csapak@proxmox.com>]

On 9/10/26 4:24 PM, Fiona Ebner wrote:
> Am 10.09.26 um 1:29 PM schrieb Dominik Csapak:
>> diff --git a/src/test/hotplug/aarch64/disk-add-default-machine.conf.expected b/src/test/hotplug/aarch64/disk-add-default-machine.conf.expected
>> new file mode 100644
>> index 00000000..b25f9a70
>> --- /dev/null
>> +++ b/src/test/hotplug/aarch64/disk-add-default-machine.conf.expected
> 
> Not a fan of having the expected result be structured like this. I would
> rather have separate files (or JSON), but not just loose separation with
> comments.

ok, see my comment further below>
>> @@ -0,0 +1,28 @@
>> +# recorded actions
>> +storage activate_volumes ["local:8006/vm-8006-disk-1.qcow2"]
>> +qmp object-add {"id":"iothread-virtio1","qom-type":"iothread"}
>> +qmp object-del {"id":"throttle-drive-virtio1"}
>> +qmp object-add {"id":"throttle-drive-virtio1","limits":{},"qom-type":"throttle-group"}
>> +qmp blockdev-add {"detect-zeroes":"on","discard":"ignore","driver":"throttle","file":{"cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"qcow2","file":{"aio":"io_uring","cache":{"direct":true,"no-flush":false},"detect-zeroes":"on","discard":"ignore","driver":"file","filename":"/var/lib/vz/images/8006/vm-8006-disk-1.qcow2","node-name":"ef4c217a7f08cf59741edc1790023f5","read-only":false},"node-name":"ff4c217a7f08cf59741edc1790023f5","read-only":false},"node-name":"drive-virtio1","read-only":false,"throttle-group":"throttle-drive-virtio1"}
>> +hmp device_add driver=virtio-blk-pci,drive=drive-virtio1,id=virtio1,bus=pcie.0,addr=0xb,iothread=iothread-virtio1,write-cache=on
>> +
>> +# hotplug errors
> 
> This one could live in the test definition already, similar to how it's
> done for the cfg2cmd tests. Then expected failures are easily
> recognizable directly.

makes sense>
>> +
>> +# differences to a freshly started VM
> 
> This should always be empty in absence of bugs. I know you used it to
> show off the fix for the iothread one, but I'd rather have the test
> runner check and enforce this directly and not record anything in the
> expected outputs. Otherwise, we have these extra lines in all test cases
> for no real benefit.

ok, i thought we maybe want to record this behavior in case we need to
keep it for backwards compatibility, but i guess this won't happen

will upgrade it to a failure of the test (and squash the
regression test into the fix)

> 
>> +
>> +# resulting config
>> +arch: aarch64
>> +bios: ovmf
>> +bootdisk: scsi0
>> +cores: 2
>> +efidisk0: local:8006/vm-8006-disk-2.qcow2,efitype=4m
>> +memory: 2048
>> +name: hotplug
>> +net0: virtio=A2:C0:43:77:08:A0,bridge=vmbr0
>> +ostype: l26
>> +scsi0: local:8006/vm-8006-disk-0.qcow2,size=32G
>> +scsihw: virtio-scsi-pci
>> +smbios1: uuid=7b10d7af-b932-4c66-b2c3-3996152ec465
>> +sockets: 1
>> +virtio1: local:8006/vm-8006-disk-1.qcow2,iothread=1,size=32G
>> +vmgenid: c773c261-d800-4348-9f5d-167fadd53cf8
> 
> Would just recording and the changes be more sensible to avoid lots of
> redundant information? That could also already live in the test
> definition with the test runner checking that nothing additional is
> changed. In most cases it will directly overlap with what is in
> [PENDING], so you don't even need to specify any additional expected
> changes.
> 
> What do you think?

what about not recording the resulting config at all as an expected
file, but simply check in the test if there is still a
[PENDING] section, mark tests where this is expected and
fail where this is not the case?

then we could have a simple file with the recorded qmp/hmp calls
in a list without any comment structure