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

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

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:
>>>> Unplugging a disk or SCSI controller removes the same parts that need to
>>>> be removed when hotplugging one of them failed. Factor this out into a
>>>> helper, so it can be reused for cleaning up after a failed hotplug in a
>>>> following commit.
>>>>
>>>> For that case, the helper can warn instead of die on errors, so that
>>>> the remaining parts are still removed and the original error can be
>>>> reported.
>>>>
>>>> Pass the parsed drive to qemu_deletescsihw instead of parsing it from
>>>> the config again, since a disk that failed to hotplug is still pending
>>>> and not in the config yet.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>>    src/PVE/QemuServer.pm | 46 +++++++++++++++++++++++++++++--------------
>>>>    1 file changed, 31 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
>>>> index 38a4e4f8..3811bff2 100644
>>>> --- a/src/PVE/QemuServer.pm
>>>> +++ b/src/PVE/QemuServer.pm
>>>> @@ -3854,6 +3854,32 @@ sub vm_devices_list {
>>>>        return $devices;
>>>>    }
>>>>    +# clean up disks and their controllers (if unused afterwards)
>>>> +sub cleanup_disk {
>>>
>>> The name is too generic, at least here in the QemuServer module.
>>
>> sure, any suggestions? `cleanup_disk_devices` ?
> 
> Yes, that's better. Or similar 'delete_disk_hardware'?

sounds good to me

> 
>>>
>>> Ideally, such a helper should rather go into QMPHelpers.pm or
>>> DriveDevice.pm. Will require moving a few of the other helpers too, but
>>> I think it's a good chance to do so. The iothread helpers should go into
>>> QMPHelpers.pm as they are not exclusive to drives, just our current
>>> usage of them. One issue is with the vm_devices_list() helper, which
>>> calls into Blockdev::qdev_id_to_drive_id(), but I guess
>>> qdev_id_to_drive_id() is fair to move to QMPHelpers anyway and that
>>> should avoid a cyclic dependency.
>>>
>>> With that we should save >100 lines of QemuServer.pm again :)
>>
>> sure I'll take a stab at it
>>
> 
> Thanks! Ah, and I notice there also is qemu_drivedel() which requires
> Blockdev::detach(), so that one should go into Blockdev.pm and then your
> new helper must then not go into QMPHelpers because that should not
> depend Blockdev, but DriveDevice may depend on Blockdev.
> 
>> thinking about this, we'd either have to break the recursion
>> of cleanup_disk -> qemu_deletescsihw -> vm_deviceunplug -> cleanup_disk
>>
>> somehow, or move vm_deviceunplug also to drivedevice/qmphelpers (which
>> seems wrong either way?)
>>
>> do you have any thoughts on that?
> 
> Of course things are more complicated than expected ^^
> So maybe it's not worth going for right now. I think breaking the
> recursion might be preferable, with first constructing the list of
> things we want to unplug in which order and then attempting to do so.
> But that seems very unrelated to your original series now. So keeping
> your helper in QemuServer would also be fine by me.

alright

> 
>>>
>>>> +    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...

> 
>>
>>>> +
>>>> +    $warn_or_die->(sub {
>>>> +        qemu_devicedel($vmid, $deviceid);
>>>> +        qemu_devicedelverify($vmid, $deviceid, $timeout);
>>>> +    });
>>>> +    $warn_or_die->(sub { qemu_drivedel($vmid, $deviceid); }) if
>>>> $deviceid =~ m/^(virtio|scsi)\d+$/;
>>>> +    # the iothread of a virtio-scsi-single disk belongs to its
>>>> controller and is removed there
>>>> +    $warn_or_die->(sub { qemu_deletescsihw($conf, $vmid,
>>>> $device); }) if $deviceid =~ m/^scsi\d+$/;
>>>> +    $warn_or_die->(sub { qemu_iothread_del($vmid, $deviceid); })
>>>> +        if $deviceid =~ m/^virtio(scsi)?\d+$/;
>>>> +
>>>> +    return;
>>>> +}
>>>> +