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

From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Fiona Ebner <f.ebner@proxmox.com>,
	Thomas Lamprecht <t.lamprecht@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 13:09:08 +0200 (CEST)	[thread overview]
Message-ID: <510878923.207.1751368148105@webmail.proxmox.com> (raw)
In-Reply-To: <52125694-2521-4e90-84d8-0a4b96990820@proxmox.com>

> Fiona Ebner <f.ebner@proxmox.com> hat am 01.07.2025 13:01 CEST geschrieben:
> 
>  
> Am 01.07.25 um 11:28 schrieb Thomas Lamprecht:
> > Am 26.06.25 um 16:40 schrieb Fiona Ebner:
> >> +
> >> +=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.

we discussed this in the past - this coupling is already there (see below), and
either qemu-server needs to hard-code storage things (which excludes third party
plugins), or pve-storage needs to encode some qemu things (the option we chose,
since there was already precedent and QEMU does provide a somewhat stable
interface there).

> > 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.
> 
> It would seem pretty strange to me that an option for accessing an image
> would depend on the machine version. We already rely on having an image
> accessed via different drivers/options to be fully equivalent for
> migration (or we couldn't combine block mirror and migration). OTOH, the
> machine version is the natural guard and there is less potential for a
> plugin to break live migration by accidentally setting or changing an
> option it shouldn't have. And if a plugin depends on a specific binary
> version for a feature, it can just also guard with the machine version
> corresponding to that binary version. So I'd just pass along the machine
> version and not the binary 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.

this is also how plugins already work:
- path returns arbitrary drive options (if it's not an actual file path)
- plugins are 100% trusted and executed in root context already, so there isn't
  much we can safeguard against..

> Sorry, I don't understand what you mean here.
> 
> Verify the block device options in the return value that the plugin
> implementation has given us? I don't think verifying would give us much,
> as QEMU will already complain the same way.
> 
> Plugins should just use the most basic options to access/open the image.
> Other options will be set by qemu-server. The POD mentions this, but
> maybe it should be emphasized more?

> >> +
> >> +    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?
> 
> What extra options do you mean? The default implementation here only
> sets driver and filename which is the most minimal possible.

since this is not 100% obvious - this default implementation here is 100% backwards
compatible for plugins that previously returned a file or block device path. it's
just plugins that already return custom options that require their own implementation.

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