From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 1FEE21FF09B for ; Mon, 14 Sep 2026 10:05:28 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D98CD214F5; Mon, 14 Sep 2026 10:05:25 +0200 (CEST) Message-ID: <17c26d82-8068-4a5b-8120-a8507fc34319@proxmox.com> Date: Mon, 14 Sep 2026 10:05:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server 1/6] tests: hotplug: add initial hotplug test harness To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260910110832.2822954-1-d.csapak@proxmox.com> <20260910110832.2822954-2-d.csapak@proxmox.com> <13cfa96c-cf55-4d94-a05a-0326e528106b@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1789373108366 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.637 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: LW3Y4WL4TSOYO5676ATPBIEQLF6CDLJB X-Message-ID-Hash: LW3Y4WL4TSOYO5676ATPBIEQLF6CDLJB X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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.