Re: [pve-devel] [PATCH v3 storage 1/9] plugin: add method to get qemu blockdevice options for volume

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Fiona Ebner <f.ebner@proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 storage 1/9] plugin: add method to get qemu blockdevice options for volume
Date: Tue, 1 Jul 2025 11:28:49 +0200	[thread overview]
Message-ID: <99f545d0-b00b-4216-803b-51fe5eac17f3@proxmox.com> (raw)
In-Reply-To: <20250626144644.279679-2-f.ebner@proxmox.com>

Am 26.06.25 um 16:40 schrieb Fiona Ebner:
> This is in preparation to switch qemu-server from using '-drive' to
> the modern '-blockdev' in the QEMU commandline options as well as for
> the qemu-storage-daemon, which only supports '-blockdev'. The plugins
> know best what driver and options are needed to access an image, so
> a dedicated plugin method returning the necessary parameters for
> '-blockdev' is the most straight-forward.
> 
> There intentionally is only handling for absolute paths in the default
> plugin implementation. Any plugin requiring more needs to implement
> the method itself. With PVE 9 being a major release and most popular
> plugins not using special protocols like 'rbd://', this seems
> acceptable.

> 
> For NBD, etc. qemu-server should construct the blockdev object.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v3:
> * Mention that volume is activated before function is called in docs.
> 
>  src/PVE/Storage.pm        | 17 +++++++++++
>  src/PVE/Storage/Plugin.pm | 62 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 

> +=pod
> +
> +=head3 qemu_blockdev_options
> +
> +    $blockdev = $plugin->qemu_blockdev_options($scfg, $storeid, $volname)
> +
> +Returns a hash reference with the basic options needed to open the volume via QEMU's C<-blockdev>
> +API. This at least requires a C<< $blockdev->{driver} >> and a reference to the image, e.g.
> +C<< $blockdev->{filename} >> for the C<file> driver. For files, the C<file> driver can be used. For
> +host block devices, the C<host_device> driver can be used. The plugin must not set options like
> +C<cache> or C<aio>. Those are managed by qemu-server and will be overwritten. For other available
> +drivers and the exact specification of the options, see
> +L<https://qemu.readthedocs.io/en/master/interop/qemu-qmp-ref.html#object-QMP-block-core.BlockdevOptions>
> +
> +While Perl does not have explicit types, the result will need to be converted to JSON later and
> +match the QMP specification (see link above), so implicit types are important. In the return value,
> +use C<JSON::true> and C<JSON::false> for booleans, C<"$value"> for strings, and C<int($value)> for
> +integers.
> +
> +The volume is activated before the function is called.
> +
> +Arguments:
> +
> +=over
> +
> +=item C<$scfg>
> +
> +The hash reference with the storage configuration.
> +
> +=item C<$storeid>
> +
> +The storage ID.
> +
> +=item C<$volume>
> +
> +The volume name.

bit much POD noise for my taste, couldn't the item's be at least placed
on a single line? E.g.:

=item C<$volume>: The volume name.
=item ...

> +
> +=back
> +
> +=cut
> +
> +sub qemu_blockdev_options {
> +    my ($class, $scfg, $storeid, $volname) = @_;

might make sense to pass some versioning information here, as otherwise this
is a bit to tight coupling for my taste and might cause long term maintenance
headache.

It might be potentially enough to have the QEMU version and machine version
passed here, as then one can generate block dev options that can be understood
by qemu(-server) and are also compatible with the target machine version.

And are we sure that we want to allow passing arbitrary options here?
Could we at least generate some simple schema automatically from the QAPI or
the like? Could be also a specially prepared JSON file just for this purpose
shipped by our qemu package, or tracked separately so that we can track the
version in which a option first appeared or became obsolete.

> +
> +    my $blockdev = {};
> +
> +    my ($path) = $class->filesystem_path($scfg, $volname);
> +
> +    if ($path =~ m|^/|) {
> +        # The 'file' driver only works for regular files. The check below is taken from
> +        # block/file-posix.c:hdev_probe_device() in QEMU. Do not bother with detecting 'host_cdrom'
> +        # devices here, those are not managed by the storage layer.
> +        my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";
> +        my $driver = (S_ISCHR($st->mode) || S_ISBLK($st->mode)) ? 'host_device' : 'file';
> +        $blockdev = { driver => $driver, filename => $path };
> +    } else {
> +        die "storage plugin doesn't implement qemu_blockdev_options() method\n";
> +    }

Should we rather default to an empty set of extra options? At least for external
plugins that would be the safer choice for upgrading, might not always work but
as is users can only loose FWICT?

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel