> Fiona Ebner <f.eb...@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
