From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 0A5721FF16B for ; Tue, 1 Jul 2025 13:01:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0143B3B8AF; Tue, 1 Jul 2025 13:02:02 +0200 (CEST) Message-ID: <52125694-2521-4e90-84d8-0a4b96990820@proxmox.com> Date: Tue, 1 Jul 2025 13:01:28 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20250626144644.279679-1-f.ebner@proxmox.com> <20250626144644.279679-2-f.ebner@proxmox.com> <99f545d0-b00b-4216-803b-51fe5eac17f3@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <99f545d0-b00b-4216-803b-51fe5eac17f3@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.149 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.237 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v3 storage 1/9] plugin: add method to get qemu blockdevice options for volume X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 01.07.25 um 11:28 schrieb Thomas Lamprecht: > Am 26.06.25 um 16:40 schrieb Fiona Ebner: >> +=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 ... Sure, fine by me, but blank lines between two =item elements seem to be necessary. >> + >> +=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. 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. 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. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel