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 E48C07A670 for ; Tue, 5 Jul 2022 09:18:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DA7522C338 for ; Tue, 5 Jul 2022 09:18:59 +0200 (CEST) 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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 5 Jul 2022 09:18:59 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0992740F4C for ; Tue, 5 Jul 2022 09:18:59 +0200 (CEST) Date: Tue, 5 Jul 2022 09:18:58 +0200 From: Wolfgang Bumiller To: Daniel Tschlatscher Cc: pve-devel@lists.proxmox.com Message-ID: <20220705071858.hfawkf64omnurqwp@casey.proxmox.com> References: <20220617104001.223893-1-d.tschlatscher@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220617104001.223893-1-d.tschlatscher@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.297 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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. [lxc.pm] Subject: [pve-devel] applied: [PATCH container v2] fix: cloning a locked container creates an empty config 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: Tue, 05 Jul 2022 07:19:00 -0000 applied, *sigh* We should probably think about getting rid functions that set a lock and return, requiring the caller to call a cleanup, *in general* in all of our perl code. Scope guards or functions taking closures would be somewhat safer. However, since this is perl, a `die` can get thrown in at *any* line (eg from a timer signal handler), and fixing that is a different beast... On Fri, Jun 17, 2022 at 12:40:01PM +0200, Daniel Tschlatscher wrote: > When an attempt was made to clone a locked container the API would > correctly present the error 'CT is locked (disk)' but create the > config files for the new container anyway. > > There was also a potential problem when the config of the new ct would > already be present and the creation of the container failed. In this > case the config of the new CT would be incorrectly removed. > The config locks for the new and the old configs should now be > correctly released depending on from which call a problem originates. > > Futhermore, I moved some related function calls into the eval block to > avoid similar problems with leftover config files in the future. > > Signed-off-by: Daniel Tschlatscher > --- > Changes from v1 > Some problems which I was made aware of by Fabian (G) through a quick > chat off-list (thanks btw!): > > * Added a dedicated check to conditionally remove the locks for the > new and old config files depending on where a potential error > originates. > * Moved some potentially failing function calls into the eval block > > src/PVE/API2/LXC.pm | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > index 64724cb..06f759e 100644 > --- a/src/PVE/API2/LXC.pm > +++ b/src/PVE/API2/LXC.pm > @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({ > my $vollist = []; > my $running; > > - PVE::LXC::Config->create_and_lock_config($newid, 0); > - PVE::Firewall::clone_vmfw_conf($vmid, $newid); > - > my $lock_and_reload = sub { > my ($vmid, $code) = @_; > return PVE::LXC::Config->lock_config($vmid, sub { > @@ -1477,14 +1474,26 @@ __PACKAGE__->register_method({ > > my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk'); > > - $running = PVE::LXC::check_running($vmid) || 0; > + eval { > + PVE::LXC::Config->create_and_lock_config($newid, 0); > + }; > + if (my $err = $@) { > + eval { PVE::LXC::Config->remove_lock($vmid, 'disk') }; > + warn "Failed to remove source CT config lock - $@\n" if $@; > > - my $full = extract_param($param, 'full'); > - if (!defined($full)) { > - $full = !PVE::LXC::Config->is_template($src_conf); > + die $err; > } > > eval { > + $running = PVE::LXC::check_running($vmid) || 0; > + > + my $full = extract_param($param, 'full'); > + if (!defined($full)) { > + $full = !PVE::LXC::Config->is_template($src_conf); > + } > + > + PVE::Firewall::clone_vmfw_conf($vmid, $newid); > + > die "parameter 'storage' not allowed for linked clones\n" > if defined($storage) && !$full; > > -- > 2.30.2