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

From: Fiona Ebner <f.ebner@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 storage 1/9] plugin: add method to get qemu blockdevice options for volume
Date: Wed, 2 Jul 2025 10:32:32 +0200	[thread overview]
Message-ID: <b64734ef-cb79-42f1-a206-df444411a355@proxmox.com> (raw)
In-Reply-To: <c74de883-2362-4f6c-90bf-2c05e78fe7f0@proxmox.com>

Am 02.07.25 um 10:12 schrieb Thomas Lamprecht:
> Am 01.07.25 um 13:01 schrieb Fiona Ebner:
>> Am 01.07.25 um 11:28 schrieb Thomas Lamprecht:
> 
>>> 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.
> 
> How so? New QEMU learns a new option for accessing images that we want to
> adopt but can't do so transparently because it's not backwards compatible?

It's much more likely that that depends only on the binary version, not
on the machine version. Again, I'm talking about "accessing an image",
which should not be any concern to how the virtual hardware looks like.
I'm not talking about options for front-end drive devices here.

>> 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).
> 
> Well, that's the current assumption that worked out mostly by luck and
> because we kept to standard options, that doesn't have to stay that way
> ant this interface would allow.

No, that's not luck. Again, you could not use block mirror in
combination with migration otherwise, because that necessarily changes
the command line. But the command line for the back-end block device
should not have any influence on the migration stream. Only the
front-end drive device matters.

>>> 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.
>>
>> Sorry, I don't understand what you mean here.
> 
> You talk about this being QEMU block dev options, so are they or are they
> mapped by us in qemu-server? If the former I'd need a more specific question,
> with the latter this is still not ideal but doesn't matter much for now.

No, they are not mapped by us.

>> 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.
> 
> What QEMU understands and what we want to expose and allow might be very
> different things.
>
>> 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?
> 
> Currently, plugins cannot set options directly at all, with this series, they
> can set anything we do not explicitly overwrite afterward in qemu-server IIUC.
> Going back from that would need a major break of the storage API. And given
> that we would like to minify the attack surface of QEMU/KVM in the future (not
> having all mounts/blockdevs/..) accessible, I could easily imagine that we do
> not want to expose all blockdev options just because they are there, starting
> out with a smaller set would easily allow this, especially if it's available
> for plugins to check if using an option is supported/allowed; that would
> have little implementation cost while reducing friction in managing this
> API surface thus reducing also maintenance cost for both us and those
> maintaining plugins.
> 
> If unsure, and I really have not seen any actual justification presented why
> such an API should be required, let's please start out with a more limiting API
> first, as for APIs allowing more flexibility is always simpler than restricting
> it later on.

Okay, see also my other reply:
https://lore.proxmox.com/pve-devel/00842c28-b265-434e-9b42-2874c6195399@proxmox.com/

>>>> +    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.
>>
> 
> The default implementation die's if it's not a path based.. So any existing
> external plugin that isn't path based will break with this; I said it a lot
> of times already, I do not want unnecessary breaking changes for the storage
> API..

Okay, we can add conversion of common protocol paths too. This is
partially at odds with restricting allowed options, because what if a
plugin sets an option in the protocol path, but we don't want to allow
that option?

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