From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id D54231FF164 for <inbox@lore.proxmox.com>; Fri, 31 Jan 2025 10:36:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E9B9328C50; Fri, 31 Jan 2025 10:36:53 +0100 (CET) Message-ID: <6366fe87-8801-4a7c-be5b-31ea4fba166a@proxmox.com> Date: Fri, 31 Jan 2025 10:36:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Fiona Ebner <f.ebner@proxmox.com> To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Daniel Herzig <d.herzig@proxmox.com> References: <20250130113121.157273-1-d.herzig@proxmox.com> <20250130113121.157273-3-d.herzig@proxmox.com> Content-Language: en-US In-Reply-To: <20250130113121.157273-3-d.herzig@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.048 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server v3 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> Am 30.01.25 um 12:31 schrieb Daniel Herzig: > The function `eject_nonrequired_isos` checks on whether a cdrom drive is > marked as required for booting the VM or not. If the parameter `essential` is not > defined, it will assume `essential` to be true and keep the current > behaviour. > > If `required` is set to 0, the function 'ejects' the ISO file by > setting the drive's file value to 'none', if the underlying storage is > unavailable or if the defined file is unavailable for another reason. > > The function is meant to be called from `config_to_command` while it > iterates over all volumes to allow for early storage activation and > early exit in the case of missing required files or unavailable > network storages. > > This commit also adds a small helper sub `file_exists` to facilitate > mocking of file existence, which is needed for testing > `eject_nonrequired_isos` once in place. > --- > PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index 808c0e1c..d151c322 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -8797,4 +8797,39 @@ sub delete_ifaces_ipams_ips { > } > } > > +sub eject_nonrequired_isos { > + my ($ds, $drive, $vmid, $storecfg, $conf) = @_; > + > + # exclude cloudinit drives from processing (parameter '1') > + return if ( !drive_is_cdrom($drive, 1) || > + $drive->{file} eq 'none' || > + $drive->{file} eq 'cdrom' ); Style nit: post-ifs should not be multiline, post-ifs don't use additional parentheses: https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If > + > + $drive->{essential} = 1 if !defined($drive->{essential}); This should rather be done when parsing the drive. > + my $iso_volid = $drive->{file}; Nit: variable could be moved up so that the previous check can already use it > + my $iso_path = PVE::QemuServer::Drive::get_iso_path($storecfg, $vmid, $iso_volid); > + my $store_err; > + if ($iso_volid !~ m|^/|) { > + my $iso_storage = PVE::Storage::parse_volume_id($iso_volid, 1); > + eval { PVE::Storage::activate_storage($storecfg, $iso_storage); }; > + $store_err = $@; I'd issue a warning here if there was a failure to activate. > + } > + > + return if ( !$store_err && > + file_exists($iso_path) ); > + > + if ($drive->{essential}) { > + die "'${ds}: ${iso_volid}': storage unavailable or file does not exist\n"; > + } else { > + log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file does not exist"); Technically, this does not eject anything, just changes the configuration. > + $drive->{file} = 'none'; > + $conf->{$ds} = print_drive($drive); Should we also make sure to write out the modified config? Especially for live-migration it's important to stay in-sync and so the migration target should not suddenly have a CD-ROM again if it became accessible again. Writing the config can also be done by the caller of course if it better fits there. > + } > +} > + > +sub file_exists { > + my $file_path = shift; > + return -e $file_path This will only work for file-based storages. The PVE::Storage::path() function can also return a protocol-based path like "glusterfs://xyz" which will not work for "-e" checks. (Our glusterfs plugin does so only for content type "images", but nothing requires a "media=cdrom" drive to be of content type "iso" right now, and I'm making a general point here, there are third-party plugins too). So to cleanly solve this, it will be required to either use e.g. "qemu-img info" as a proxy for existence checking, which is often done in our code via volume_size_info() (or alternatively to add a dedicated storage+plugin API function to check for the existence of an image). > +} > + > 1; _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel