From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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 4BF0F69397
for <pve-devel@lists.proxmox.com>; Tue, 23 Feb 2021 15:50:19 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 4218F1FBA2
for <pve-devel@lists.proxmox.com>; Tue, 23 Feb 2021 15:49:49 +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 7FA3E1FB91
for <pve-devel@lists.proxmox.com>; Tue, 23 Feb 2021 15:49:47 +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 43989462D1
for <pve-devel@lists.proxmox.com>; Tue, 23 Feb 2021 15:49:47 +0100 (CET)
Message-ID: <be41758d-83be-a6a8-d3f6-0324e9ce70dc@proxmox.com>
Date: Tue, 23 Feb 2021 15:49:45 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:86.0) Gecko/20100101
Thunderbird/86.0
Content-Language: en-US
To: Oguz Bektas <o.bektas@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20210223122936.662855-1-o.bektas@proxmox.com>
<72b18013-81ac-35a5-0055-d76297ff02ec@proxmox.com>
<20210223143640.GB10131@gaia.proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <20210223143640.GB10131@gaia.proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.056 Adjusted score from AWL reputation of From: address
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.001 Looks like a legit reply (A)
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 <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>
X-List-Received-Date: Tue, 23 Feb 2021 14:50:19 -0000
On 23.02.21 15:36, Oguz Bektas wrote:
> 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 <o.bektas@proxmox.com>
>>> ---
>>> 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
CLI is wrong, it's API (CLI is just derived from API).
>
> otherwise i think it's not super obvious what's going on, since this
> part of the code has a lot of special cases
>
the "whats going on" is obvious, and a why is rather uncontroversial here, missing this
was a clear bug.
To me this is like commenting a
# add orig comment if not api comment
$comment = $orig_comment if !defined($api_comment) && defined($orig_comment);
or almost like
# set a to 1 if $b is true
$a = 1 if $b;
which gets one to commenting every da+#!n value assignment? no thanks!
>>
>>
>>> + $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) {
>>>
>>