From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D81459229F for ; Fri, 16 Feb 2024 16:56:38 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AC4775C02 for ; Fri, 16 Feb 2024 16:56:38 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 16 Feb 2024 16:56:37 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B2D0B4855B for ; Fri, 16 Feb 2024 16:56:37 +0100 (CET) Message-ID: <7f8f7839-37d5-4531-8c70-559968c970f9@proxmox.com> Date: Fri, 16 Feb 2024 16:56:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Philipp Hufnagl References: <20231127114001.189590-1-p.hufnagl@proxmox.com> <20231127114001.189590-5-p.hufnagl@proxmox.com> From: Fiona Ebner In-Reply-To: <20231127114001.189590-5-p.hufnagl@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.071 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [config.pm, pbsplugin.pm] Subject: Re: [pve-devel] [PATCH storage v4 4/4] pbs: fix #5008: Check if datastore and namespace is valid on add- and update hooks 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: , X-List-Received-Date: Fri, 16 Feb 2024 15:56:38 -0000 Am 27.11.23 um 12:40 schrieb Philipp Hufnagl: > This adds a check if the datastore and the namespace is valid when a > user attempts to add a new PBS datastore. > > Since the namespace only can be checked after the datastore is > validated, the datastore will be checked as well, regardless that it > will be done later in the superclass anyway. > > The functionallity to check namespaces is added with this commit. For > checking the datastore, existing code that has previously been > refactored will be reused. > > Because the server address is needed to check the namespaces in the > update hook, it has to be included in the Storage/Config.pm. Rather than "in the Storage/Config.pm" I suppose you mean "in the > > Signed-off-by: Philipp Hufnagl > --- > src/PVE/API2/Storage/Config.pm | 4 ++-- > src/PVE/Storage/PBSPlugin.pm | 41 ++++++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm > index e04b6ab..651d4bc 100755 > --- a/src/PVE/API2/Storage/Config.pm > +++ b/src/PVE/API2/Storage/Config.pm > @@ -362,12 +362,12 @@ __PACKAGE__->register_method ({ > } > } > > - $returned_config = $plugin->on_update_hook($storeid, $opts, %$sensitive); > - > for my $k (keys %$opts) { > $scfg->{$k} = $opts->{$k}; > } > > + $returned_config = $plugin->on_update_hook($storeid, $scfg, %$sensitive); > + This hunk should be its own patch. Hmm, could be fine in practice, but there is potential for breakage. Existing (third-party) plugins might be relying on the fact that the hook is only called with updated values somehow? On the other hand, currently our CIFS plugin prints a warning that the updated password will be ignored, because no user is set if the user is not also updated at same time but does exist in the current config. Luckily the actual setting of the credentials is not affected. This change would correct this. > if (defined($scfg->{mkdir})) { # TODO: remove complete option in Proxmox VE 9 > warn "NOTE: The 'mkdir' option set for '${storeid}' is deprecated and will be removed" > ." in Proxmox VE 9. Use 'create-base-path' or 'create-subdirs' instead.\n" ---snip--- > @@ -833,6 +855,25 @@ sub check_datastore_exists { > die "$storeid: Cannot find datastore '$datastore', check permissions and existence!\n"; > } > > +sub check_namespace_exists { Because it dies when the namespace doesn't exist, I'd prefer to call it assert_ rather than check_ > + my ($class, $storeid, $scfg, $password, $conn) = @_; > + > + my $namespace = $scfg->{namespace}; > + return 1 if !defined($namespace); > + my $datastore = $scfg->{datastore}; Style nit: rather than having this one-time-use variable, please use $scfg->{datastore} directly below. > + > + my $namespaces = eval { scan_namespaces($scfg, $datastore, $password, $conn) }; > + die "$storeid: $@" if $@; > + > + for my $ns (@$namespaces) { > + if ($ns->{ns} eq $namespace) { > + return 1; > + } Style nit: would save two lines using post-if > + } > + > + die "$storeid: Cannot find namespace '$namespace', check permissions and existence!\n"; > +} > + > sub activate_storage { > my ($class, $storeid, $scfg, $cache) = @_; >