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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6534069394 for ; Tue, 23 Feb 2021 15:38:50 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 53E0D1FA12 for ; Tue, 23 Feb 2021 15:38:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id 6C2FF1FA08 for ; Tue, 23 Feb 2021 15:38:49 +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 2CB93462B8 for ; Tue, 23 Feb 2021 15:38:49 +0100 (CET) Date: Tue, 23 Feb 2021 15:36:40 +0100 From: Oguz Bektas To: Thomas Lamprecht Cc: Proxmox VE development discussion Message-ID: <20210223143640.GB10131@gaia.proxmox.com> Mail-Followup-To: Oguz Bektas , Thomas Lamprecht , Proxmox VE development discussion References: <20210223122936.662855-1-o.bektas@proxmox.com> <72b18013-81ac-35a5-0055-d76297ff02ec@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <72b18013-81ac-35a5-0055-d76297ff02ec@proxmox.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-SPAM-LEVEL: Spam detection results: 0 AWL 1.514 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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, lxc.pm] Subject: Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore 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, 23 Feb 2021 14:38:50 -0000 hi, On Tue, Feb 23, 2021 at 03:21:28PM +0100, Thomas Lamprecht wrote: > On 23.02.21 13:29, Oguz Bektas wrote: > > since pct defaults to privileged containers, it restores the container > > as privileged when `--unprivileged 1` is not passed. > > > > instead we should check the old configuration and retrieve it > > from there. > > > > this way, when one creates an unprivileged container on GUI, it will be > > still restored as unprivileged via pct (without having to pass > > `--unprivileged 1` parameter) > > > > please note the effects of your change to `if ($is_root && $archive ne '-') {` > Fabi describes, pick up his R-b/T-b tag and send a v3 with the style comments > below addressed. will do > > > Signed-off-by: Oguz Bektas > > --- > > v1->v2: > > * move the $is_root guard > > * wrap line to make it shorter > > * shorten comment > > * use () around defined > > * also check defined($orig_conf->{unprivileged}) > > > > > > src/PVE/API2/LXC.pm | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > > index 8ce462f..3d3dbb0 100644 > > --- a/src/PVE/API2/LXC.pm > > +++ b/src/PVE/API2/LXC.pm > > @@ -352,7 +352,7 @@ __PACKAGE__->register_method({ > > my $orig_mp_param; # only used if $restore > > if ($restore) { > > die "can't overwrite running container\n" if PVE::LXC::check_running($vmid); > > - if ($is_root && $archive ne '-') { > > + if ($archive ne '-') { > > my $orig_conf; > > print "recovering backed-up configuration from '$archive'\n"; > > ($orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($storage_cfg, $archive, $vmid); > > @@ -361,7 +361,11 @@ __PACKAGE__->register_method({ > > # causing it to restore the raw lxc entries, among which there may be > > # 'lxc.idmap' entries. We need to make sure that the extracted contents > > # of the container match up with the restored configuration afterwards: > > - $conf->{lxc} = $orig_conf->{lxc}; > > + $conf->{lxc} = $orig_conf->{lxc} if $is_root; > > + > > + # make sure to retrieve the privilege level of container if not specified > > Does this really adds any value in your opinion? It IMO adds even some confusion > as its not clear where has to be "not specified"... I'd really just drop it. i adapt it to: # retrieve the privilege level of container if cli parameter was not passed otherwise i think it's not super obvious what's going on, since this part of the code has a lot of special cases > > > > + $conf->{unprivileged} = $orig_conf->{unprivileged} if !defined($unprivileged) > > + && defined($orig_conf->{unprivileged}); > > that's not how we wrap lines for post ifs, as you can se from looking at any code > of ours... > > Wrote it now also down as more definite rule in the Perl Style Guide > https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If ok thank you > > > } > > } > > if ($storage_only_mode) { > > >