Re: [PATCH qemu-server v5 3/4] hotplug: factor out cleanup of disk devices

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

Am 03.11.26 um 4:11 PM schrieb Dominik Csapak:
> On 11/3/26 3:41 PM, Fiona Ebner wrote:
>> Am 03.11.26 um 2:01 PM schrieb Dominik Csapak:
>>> On 11/3/26 12:06 PM, Fiona Ebner wrote:
>>>> Am 30.10.26 um 11:11 AM schrieb Dominik Csapak:
>>>>
>>>>> +    my ($conf, $vmid, $deviceid, $device, $timeout, $noerr) = @_;
>>>>> +
>>>>> +    my $warn_or_die = sub {
>>>>> +        my ($sub) = @_;
>>>>> +        eval { $sub->(); };
>>>>> +        if (my $err = $@) {
>>>>> +            die $err if !$noerr;
>>>>> +            warn $err;
>>>>> +        }
>>>>> +    };
>>>>
>>>> Rather than having this with $noerr, just make the callers use our
>>>> common eval + warn pattern. That would improve readability below and on
>>>> the call sites (reading a "1" there won't tell you how it relates to
>>>> error behavior without looking it up).
>>>>
>>>
>>> If i understand you correctly this would change the semantic though?
>>>
>>> now with noerr = 1, every cleanup here runs regardless if it fails or
>>> not.
>>>
>>> if i just check on the call site, the first error skips the cleanup
>>> of the rest?
>>
>> Right, but isn't that actually what we want? The objects depend on each
>> other, so we should only delete them in order. We do/should not fail if
>> an object is already deleted. Otherwise your next patch would introduce
>> a bunch of additional warnings in most error cases when attempting to
>> remove stuff that was never added.
> 
> well it might happen that devicedelverify is failing even though
> it's actually removed if i understand https://bugzilla.proxmox.com/
> show_bug.cgi?id=8273 correctly?
> 
> so we might want to continue anyway? they shouldn't be removable
> if something depends on it, but trying in any case works around
> some timeouts i think...
If devicedelverify() reports that it failed, we should assume so. It
would just be luck if the subsequent command works because of timing
reasons.

We should make sure a subsequent cleanup removes the left-overs, but
that is independent.

I would rather bump the timeout for the relevant 'query-pci' command
(not too much if not inside a worker context) to avoid the failure in
the first place.