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 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'?

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

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

> 
>>> +
>>> +    $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;
>>> +}
>>> +