Re: [PATCH qemu-server 1/6] tests: hotplug: add initial hotplug test harness

[Fiona Ebner <f.ebner@proxmox.com>]

Am 11.09.26 um 12:08 PM schrieb Dominik Csapak:
> On 9/10/26 4:24 PM, Fiona Ebner wrote:
>> Am 10.09.26 um 1:09 PM schrieb Dominik Csapak:
> [snip]>> +
>>> +# the hotplug helpers wait between retries when verifying
>>> (un)plugged devices, do not wait in tests
>>> +BEGIN {
>>> +    *CORE::GLOBAL::sleep = sub { return 0; };
>>> +}
>>
>> Why is this necessary in the tests? We control whether adding or
>> removing a device works in the tests, so I feel like this sweeps
>> something under the rug that we could improve in qemu-server. Does
>> qemu-server (sometimes) call {add,del}verify() when {add,del}() failed?
> 
> 
> The issue here is the hmp device_add call does not fail directly,
> but returns the text (which can contain an error) so we always
> call verify after (and when testing the hotplug issues etc. this is what
> fails, not the device_add)
> 
> IMO in the long-term we should not use the hmp calls at all which would
> clean this up a bit, but in the meantime not overriding the sleep here
> just makes the tests longer for the variants where the adding fails..
> 
> Not sure what exactly can come back from a hmp call, so treating all
> non-empty returned strings as an error has a bit of regression potential.

I suppose we could match for 'Error:' since hmp_handle_error() adds that
prefix. But fully agree, in the long run, not using
human-monitor-command, but the direct QMP calls is better :)

I'm fine with keeping the hack, but please add a comment.

> 
> 
> [snip]>> +
>>> +# The buses a machine provides on its own. Everything else (PCI
>>> bridges, SCSI and USB controllers)
>>> +# comes from devices on the command line or from the config files
>>> read via -readconfig.
>>> +sub machine_buses($conf, $machine) {
>>
>> default_buses_for_machine(). Can't we somehow get this from qemu-server?
>> I'd like to avoid the need to duplicate/hard-code this here.
>>
> 
> aside from starting a vm and querying with qmp (which i think
> is overkill) i'm not aware of any way how to get that info out
> of qemu.

Okay, fair :)

> 
>>> +    return { map { $_ => 1 } qw(pcie.0) } if $machine =~ m/^virt/;
>>> +    return { map { $_ => 1 } ('pcie.0', map { "ide.$_" } 0 .. 5) }
>>> +        if PVE::QemuServer::Machine::machine_type_is_q35($conf);
>>> +    return { map { $_ => 1 } qw(pci.0 ide.0 ide.1) };
>>> +}
>>> +
> [snip]
>>> +
>>> +# QEMU resolves the unversioned aliases 'pc', 'q35' and 'virt' to
>>> the versioned default machine of
>>> +# the running binary, which is also what query-machines reports for
>>> the running VM. The pve version
>>> +# is kept as it was requested on the command line.
>>> +sub resolve_machine_alias($machine) {
>>
>> Can't you use the windows_get_pinned_machine_version() function here?
>> I'd like to avoid duplicate fucntions for things that already exist. We
>> could also drop the 'windows_' prefix if we want, the function itself is
>> not concerned with that, we just use it only for Windows.
>>
> 
> it's hard because we get here the +pveX prefix sometimes, but
> windows_get_pinned_machine_verions does not work with that, and possibly
> adds one itself, so we'd have to check for that afterwards too
> 
> not sure this is worth it just to save on one extra aliias list?
> 
> i also don't see an obvious way to restructure the machine code
> to achieve what we want here
> 
> basically we sometimes get here 'pc+pve0' but actually want
> 'pc-i440fx-X.Y+pve0'
> 
> I'll try though

Thanks! If it doesn't work out, I'm fine with keeping the current one.