Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd

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

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 <l.nunner@proxmox.com>
> ---
> 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<version>.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;
>  }