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 CEB3B922DE for ; Mon, 12 Sep 2022 11:13:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C15D72A27 for ; Mon, 12 Sep 2022 11:13:34 +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 ; Mon, 12 Sep 2022 11:13:34 +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 CABE043076 for ; Mon, 12 Sep 2022 11:13:33 +0200 (CEST) Message-ID: Date: Mon, 12 Sep 2022 11:13:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:105.0) Gecko/20100101 Thunderbird/105.0 Content-Language: en-GB To: Proxmox VE development discussion , Leo Nunner References: <20220909114539.61612-1-l.nunner@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220909114539.61612-1-l.nunner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.900 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 NICE_REPLY_A -4.101 Looks like a legit reply (A) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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 - Subject: Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd 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: Mon, 12 Sep 2022 09:13:34 -0000 Looks ok semantically, some comments for mostly code style issues inline On 09/09/2022 13:45, Leo Nunner wrote: > Signed-off-by: Leo Nunner > --- > In newer versions, libsystemd-shared-*.so is placed in an > architecture-specific folder (e.g. /usr/lib/x86_64-linux-gnu/systemd/). > I adapted the code to include the current architecture while searching, as > well as allowing for the addition of further paths, should it change again > in the future. why is above outside the commit message's body as mail comment? This is good and important info for the patch to have tracked in git. > > src/PVE/LXC/Setup.pm | 2 +- > src/PVE/LXC/Setup/Alpine.pm | 2 +- > src/PVE/LXC/Setup/Base.pm | 33 +++++++++++++++++++++++++-------- > src/PVE/LXC/Setup/Devuan.pm | 2 +- > src/PVE/LXC/Setup/Plugin.pm | 2 +- > src/PVE/LXC/Setup/Unmanaged.pm | 2 +- > 6 files changed, 30 insertions(+), 13 deletions(-) > > diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm > index b72a18e..6446094 100644 > --- a/src/PVE/LXC/Setup.pm > +++ b/src/PVE/LXC/Setup.pm > @@ -285,7 +285,7 @@ sub post_create_hook { > sub unified_cgroupv2_support { > my ($self) = @_; > > - return $self->protected_call(sub { $self->{plugin}->unified_cgroupv2_support() }); > + return $self->protected_call(sub { $self->{plugin}->unified_cgroupv2_support($self->{conf}) }); > } > > # os-release(5): > diff --git a/src/PVE/LXC/Setup/Alpine.pm b/src/PVE/LXC/Setup/Alpine.pm > index b56d895..5d22e69 100644 > --- a/src/PVE/LXC/Setup/Alpine.pm > +++ b/src/PVE/LXC/Setup/Alpine.pm > @@ -102,7 +102,7 @@ sub setup_network { > > # non systemd based containers work with pure cgroupv2 > sub unified_cgroupv2_support { > - my ($self) = @_; > + my ($self, $conf) = @_; why pass the whole config if you just need the arch? Please avoid overly generic parameter in signatures if only one specific thing is required. > > return 1; > } > diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm > index cc12914..f58edf8 100644 > --- a/src/PVE/LXC/Setup/Base.pm > +++ b/src/PVE/LXC/Setup/Base.pm > @@ -517,20 +517,36 @@ sub clear_machine_id { > # tries to guess the systemd (major) version based on the existence of > # (/usr)?/lib/systemd/libsystemd-shared.so. It was introduced in v231. > sub get_systemd_version { > - my ($self) = @_; > + my ($self, $conf) = @_; > > - my $sd_lib_dir = $self->ct_is_directory("/lib/systemd") ? > - "/lib/systemd" : "/usr/lib/systemd"; > - my $libsd = PVE::Tools::dir_glob_regex($sd_lib_dir, "libsystemd-shared-.+\.so"); > - if (defined($libsd) && $libsd =~ /libsystemd-shared-(\d+)(?:\..*)?\.so/) { > - return $1; > + my $current_arch = $conf->{arch}; > + my %arch_full_names = ( > + "amd64" => "x86_64", > + "i386" => "i386", > + "arm64" => "aarch64", > + "armhf" => "arm" > + ); rather unrelated to the method, I'd factor it out in its own private sub my sub arch_to_full_name { my ($arch) = @_; ... } and please also handle the case where arch isn't known, e.g., by explicitly dying, to avoid gettin a less visible "undef in string concat" warning. > + > + my @search_dirs = ( > + "/lib/systemd", > + "/usr/lib/systemd", > + "/usr/lib/" . $arch_full_names{$current_arch} . "-linux-gnu/systemd/" In perl you can do string concatenation of variables inside double quotes, albeit it's a bit ugly for hash accesses, but with abofrementioned factoring out of the arch -> full name resolution it would be a simple scalar variable here anyway: "/usr/lib/${arch_full_name}-linux-gnu/systemd/" At some point it may get easier to just parse `ldd /sbin/init` though ^^ > + ); > + > + foreach my $sd_lib_dir ( @search_dirs ) { > + next if !$self->ct_is_directory($sd_lib_dir); > + > + my $libsd = PVE::Tools::dir_glob_regex($sd_lib_dir, "libsystemd-shared-.+\.so"); > + if (defined($libsd) && $libsd =~ /libsystemd-shared-(\d+)(?:\..*)?\.so/) { > + return $1; > + } > } > > return undef; > }