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

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

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.

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

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

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