Re: [PATCH qemu-server 1/2] drive device: use configured serial as SCSI device ID

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

Am 28.10.26 um 8:07 PM schrieb Michael Köppl:
> Since the switch to '-blockdev', the device ID of SCSI disks is
> explicitly set to the drive name, even if serial is configured. So with
> machine version 10.0 or newer, a SCSI disk with a serial reports the
> drive name (e.g. 'drive-scsi1') and the /dev/disk/by-id paths that udev
> derives from it no longer contain the serial. In a guest running Proxmox
> VE, the disk list API endpoint (/nodes/<node>/disks/list) therefore does
> not report the configured serial either.
> 
> Set the device ID explicitly to the serial if one is configured, like
> libvirt does since its switch to '-blockdev' [0]. For serials of up to
> 20 bytes, this is the same device ID that QEMU used by default with
> '-drive'.
> 
> Relying on QEMU's default instead would not work for all serials. QEMU
> accepts serials of up to 36 bytes [1], but only uses serials of up to
> 20 bytes as the default device ID and refuses to start the device for
> longer ones instead of truncating them [2][3].

Orthogonal to the series, but turns out, our schema allows serials
longer than that, because it has

maxLength => 20 * 3, # *3 since it's %xx url enoded

but nothing enforces that each character is url-encoded, so regular
alphanumeric characters do not contribute 3 each. On the other hand, it
still has the old limit of 20 if all characters are url-encoded.

We cannot just enforce it for existing values, as that would be a
breaking change, but we could start checking the actual decoded length
for new values (and using a limit of 36 there). But again, this is
orthogonal.

> 
> As this changes what the guest sees, only do so starting with machine
> version 11.1+pve1. Check it with the version guard, so that only VMs
> with a SCSI disk with a serial require the new revision.
> 
> [0]: https://gitlab.com/libvirt/libvirt/-/commit/a1dce96236f6d35167924fa7e6a70f58f394b23c
> [1]: https://lists.gnu.org/archive/html/qemu-stable/2016-09/msg00117.html
> [2]: https://lists.gnu.org/archive/html/qemu-block/2024-06/msg00027.html
> [3]: https://github.com/systemd/systemd/issues/39262
> 
> Fixes: 46d1225e ("drive device: fix regression with missing '/dev/disk/by-id' paths")
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> Added a machine version 11.1+pve1 specifically for this. If it makes
> more sense to wait for 11.2, considering this is not an urgent issue,
> I'll of course send a v2. Should be a bit simpler to implement anyway.

Since AFAIK, nobody except you complained yet, we might want to wait for
11.2, yes. Maybe also print a line notifying users who have 'serial'
configured and using 'latest' machine version about the upcoming change
and tell them to pin the machine version if they don't want the change?
I'm even somewhat inclined to delay it to the next major release,
because it is a guest ABI change that can have quite some fallout
unfortunately if the guest relies on the specific device path. Then we
can scan in pve9to10 for disks with serial and using 'latest' machine
type and warn accordingly.

> 
>  src/PVE/QemuServer.pm             |  7 ++++---
>  src/PVE/QemuServer/DriveDevice.pm | 22 +++++++++++++++++++---
>  src/PVE/QemuServer/Machine.pm     |  6 ++++++
>  3 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 1efeeb66..36471bdf 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -3686,9 +3686,10 @@ sub config_to_command {
>                  push @$devices, '-drive', $drive_cmd;
>              }
> 
> -            push @$devices, '-device', print_drivedevice_full(
> -                $storecfg, $conf, $vmid, $drive, $arch, $machine_type,
> -            );
> +            push @$devices, '-device',
> +                print_drivedevice_full(
> +                    $storecfg, $conf, $vmid, $drive, $arch, $machine_type, $version_guard,
> +                );
>          },
>      );
> 
> diff --git a/src/PVE/QemuServer/DriveDevice.pm b/src/PVE/QemuServer/DriveDevice.pm
> index 621fcc2b..980a217a 100644
> --- a/src/PVE/QemuServer/DriveDevice.pm
> +++ b/src/PVE/QemuServer/DriveDevice.pm
> @@ -69,13 +69,23 @@ sub get_max_scsihw_index {
>  }
> 
>  sub print_drivedevice_full {
> -    my ($storecfg, $conf, $vmid, $drive, $arch, $machine_type) = @_;
> +    my (
> +        $storecfg,
> +        $conf,
> +        $vmid,
> +        $drive,
> +        $arch,
> +        $machine_type,
> +        $version_guard, # checks the machine version and records the required +pveX revision
> +    ) = @_;
> 
>      my $device = '';
>      my $maxdev = 0;
> 
>      my $machine_version =
>          PVE::QemuServer::Machine::extract_version($machine_type, kvm_user_version());
> +    # when hotplugging, the machine is already running, so there is no required version to record
> +    $version_guard //= sub { return min_version($machine_version, @_); };

This should be at the call site for hotplug. In fact, it already is,
just needs to moved out of the branch for 'netX' to the top-level of
vm_deviceplug().

>      my $has_write_cache = 1; # whether the device has a 'write-cache' option
> 
>      my $use_blockdev = min_version($machine_version, 10, 0);
> @@ -112,9 +122,15 @@ sub print_drivedevice_full {
>          $device .= ",id=$drive_id";
> 
>          # For the switch to -blockdev, the SCSI device ID needs to be explicitly specified. Note
> -        # that only ide-cd and ide-hd have a 'device_id' option.
> +        # that only ide-cd and ide-hd have a 'device_id' option. Since machine version 11.1+pve1,
> +        # a configured serial is used as the device ID, like QEMU's default with -drive.
>          if ($use_blockdev && ($device_type eq 'cd' || $device_type eq 'hd')) {
> -            $device .= ",device_id=drive-${drive_id}";
> +            if ($drive->{serial} && $version_guard->(11, 1, 1)) {
> +                my $serial = _escape_qemu_arg_value(URI::Escape::uri_unescape($drive->{serial}));
> +                $device .= ",device_id=$serial";
> +            } else {
> +                $device .= ",device_id=drive-${drive_id}";
> +            }
>          }
> 
>          if ($drive->{ssd} && ($device_type eq 'block' || $device_type eq 'hd')) {
> diff --git a/src/PVE/QemuServer/Machine.pm b/src/PVE/QemuServer/Machine.pm
> index 35161d1f..68cc5daf 100644
> --- a/src/PVE/QemuServer/Machine.pm
> +++ b/src/PVE/QemuServer/Machine.pm
> @@ -50,6 +50,12 @@ our $PVE_MACHINE_VERSION = {
>              '+pve2' => 'Enable hv_emcs for Windows 11/2022/2025 to mitigate issues with VBS.',
>          },
>      },
> +    '11.1' => {
> +        highest => 1,
> +        revisions => {
> +            '+pve1' => 'Use the configured serial as device ID for SCSI disks.',
> +        },
> +    },
>  };
> 
>  my $machine_fmt = {
> --
> 2.47.3
> 
> 
> 
> 
>