Re: [pve-devel] [PATCH v2 container] fix #3313: recover unprivileged bit from old config during pct restore

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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) {
>>>
>>