From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C57D91FF164 for ; Fri, 17 Jan 2025 13:32:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0DF41E220; Fri, 17 Jan 2025 13:32:51 +0100 (CET) From: Daniel Herzig To: Daniel Kral In-Reply-To: <885b15ca-27f4-4a39-b7a3-4e0ddb3844d9@proxmox.com> (Daniel Kral's message of "Thu, 16 Jan 2025 16:19:03 +0100") References: <20250113085608.99498-1-d.herzig@proxmox.com> <20250113085608.99498-4-d.herzig@proxmox.com> <885b15ca-27f4-4a39-b7a3-4e0ddb3844d9@proxmox.com> Date: Fri, 17 Jan 2025 13:32:16 +0100 Message-ID: <877c6tmxin.fsf@proxmox.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.284 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.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_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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemuserver.pm] Subject: Re: [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles 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 Cc: 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" Daniel Kral writes: > On 1/13/25 09:55, Daniel Herzig wrote: >> Current behaviour prevents a VM from starting, if an ISO file defined >> in the configuration becomes unavailable. >> The function eject_nonrequired_isos checks on whether a cdrom drive >> is >> marked as 'required' or not. If the parameter 'required' is not >> defined, it will assume 'required' 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 called while config_to_command iterates over all >> volumes to allow for early storage activation and early exit in the >> case of missing required files. >> Signed-off-by: Daniel Herzig >> --- >> PVE/QemuServer.pm | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm >> index d07c170e..f72878d3 100644 >> --- a/PVE/QemuServer.pm >> +++ b/PVE/QemuServer.pm >> @@ -4041,6 +4041,8 @@ sub config_to_command { >> PVE::QemuConfig->foreach_volume($conf, sub { >> my ($ds, $drive) = @_; >> + eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf); >> + > > This change will unfortunately make two config2cmd test cases fail and > therefore the build process will also fail. It is important that the > package can be built at each individual commit so to make the package > bisectable. > > IMO this patch could be split into "introduce eject_norequired_isos" > and patches #4-#9 could be squashed and put together with adding > "eject_nonrequired_isos" to config_to_command in the same > patch. Therefore someone reviewing (now or in the future) and know > what tests needed to be added/changed when adding this function call > to config_to_command. > Thanks for pointing that out. Batch-applying the patches on my test-system I completely missed this. I'll work out something along your lines for v3. >> if (PVE::Storage::parse_volume_id($drive->{file}, 1)) { >> check_volume_storage_type($storecfg, $drive->{file}); >> push @$vollist, $drive->{file}; >> @@ -8999,6 +9001,43 @@ sub delete_ifaces_ipams_ips { >> } >> } >> +sub eject_nonrequired_isos { >> + my ($ds, $drive, $vmid, $storecfg, $conf) = @_; >> + # set 1 to exclude cloudinit. cloudinit isos are always required. >> + if (drive_is_cdrom($drive, 1) >> + && $drive->{file} ne 'none' >> + && $drive->{file} ne 'cdrom') { > > nit: IMO, this could be an early return: > > return if !drive_is_cdrom($drive, 1); > return if $drive->{file} eq 'none' || $drive->{file} eq 'cdrom'; > > So that we can reduce the following to only 2 indentation levels. > I like this idea. >> + $drive->{required} = 1 if !defined($drive->{required}); >> + my $iso_volid = $drive->{file}; >> + my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file}); > > nit: third argument could be $iso_volid > Scratching my head how I missed this :). >> + 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 = $@; >> + } >> + if ($store_err) { >> + if ($drive->{required}) { >> + die "cannot access required file: '${ds}: ${iso_volid}': ${store_err}\n"; >> + } else { >> + log_warn("eject '${ds}: ${iso_volid}': ${store_err}"); >> + $drive->{file} = 'none'; >> + $conf->{$ds} = print_drive($drive); >> + } >> + } else { >> + if (!file_exists($iso_path)) { >> + if ($drive->{required}) { >> + die "required file does not exist: '${ds}: ${iso_volid}'\n"; >> + } else { >> + log_warn("eject '${ds}: ${iso_volid}': file does not exist"); >> + $drive->{file} = 'none'; >> + $conf->{$ds} = print_drive($drive); >> + } >> + } >> + } > > nit: the logic between an unavailable storage and an unavailable ISO > image are very similar (both `$drive->{required} && $store_err` as > well as `$drive->{required} && !file_exists($iso_path)` have the same > exit control path), so we could simplify this e.g. to this (changes > the warning message to a generic message for unavailable storages > too): > > if ($drive->{required}) { > die "cannot access required file: '${ds}: ${iso_volid}': > ${store_err}\n" if $store_err; > die "required file does not exist: '${ds}: ${iso_volid}'\n" if > !file_exists($iso_path); > } > > return if !$store_err && file_exists($iso_path); > > log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file > does not exist"); > > $drive->{file} = 'none'; > $conf->{$ds} = print_drive($drive); > Good point, thanks. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel