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

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

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

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

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

>> +
>> +    $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;
>> +}
>> +
>>   sub vm_deviceplug {
>>       my ($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type) = @_;
>>   
>> @@ -3972,20 +3998,12 @@ sub vm_deviceunplug {
>>           qemu_devicedel($vmid, $deviceid);
>>           qemu_devicedelverify($vmid, $deviceid);
>>       } elsif ($deviceid =~ m/^(virtio)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid);
>> -        qemu_drivedel($vmid, $deviceid);
>> -        qemu_iothread_del($vmid, $deviceid);
>> +        cleanup_disk($conf, $vmid, $deviceid);
>>       } elsif ($deviceid =~ m/^(virtioscsi|scsihw)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid, 15);
>> -        qemu_iothread_del($vmid, $deviceid) if $deviceid =~ m/^virtioscsi\d+$/;
>> +        cleanup_disk($conf, $vmid, $deviceid, undef, 15);
>>       } elsif ($deviceid =~ m/^(scsi)(\d+)$/) {
>> -        qemu_devicedel($vmid, $deviceid);
>> -        qemu_devicedelverify($vmid, $deviceid);
>> -        qemu_drivedel($vmid, $deviceid);
>> -        # the iothread of a virtio-scsi-single disk belongs to its controller and is removed there
>> -        qemu_deletescsihw($conf, $vmid, $deviceid);
>> +        my $device = parse_drive($deviceid, $conf->{$deviceid});
>> +        cleanup_disk($conf, $vmid, $deviceid, $device);
>>       } elsif ($deviceid =~ m/^(net)(\d+)$/) {
>>           qemu_devicedel($vmid, $deviceid);
>>           qemu_devicedelverify($vmid, $deviceid);
>> @@ -4124,9 +4142,7 @@ sub qemu_findorcreatescsihw {
>>   }
>>   
>>   sub qemu_deletescsihw {
>> -    my ($conf, $vmid, $opt) = @_;
>> -
>> -    my $device = parse_drive($opt, $conf->{$opt});
>> +    my ($conf, $vmid, $device) = @_;
>>   
>>       if ($conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single')) {
>>           vm_deviceunplug($vmid, $conf, "virtioscsi$device->{index}");
>