* [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd
@ 2022-09-09 11:45 Leo Nunner
2022-09-12 9:13 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Leo Nunner @ 2022-09-09 11:45 UTC (permalink / raw)
To: pve-devel
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.
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) = @_;
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"
+ );
+
+ my @search_dirs = (
+ "/lib/systemd",
+ "/usr/lib/systemd",
+ "/usr/lib/" . $arch_full_names{$current_arch} . "-linux-gnu/systemd/"
+ );
+
+ 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;
}
sub unified_cgroupv2_support {
- my ($self) = @_;
+ my ($self, $conf) = @_;
# https://www.freedesktop.org/software/systemd/man/systemd.html
# systemd is installed as symlink to /sbin/init
@@ -542,7 +558,8 @@ sub unified_cgroupv2_support {
}
# systemd version 232 (e.g. debian stretch) supports the unified hierarchy
- my $sdver = $self->get_systemd_version();
+ my $sdver = $self->get_systemd_version($conf);
+
if (!defined($sdver) || $sdver < 232) {
return 0;
}
diff --git a/src/PVE/LXC/Setup/Devuan.pm b/src/PVE/LXC/Setup/Devuan.pm
index 3e15bb2..18cb9cf 100644
--- a/src/PVE/LXC/Setup/Devuan.pm
+++ b/src/PVE/LXC/Setup/Devuan.pm
@@ -42,7 +42,7 @@ sub new {
# non systemd based containers work with pure cgroupv2
sub unified_cgroupv2_support {
- my ($self) = @_;
+ my ($self, $conf) = @_;
return 1;
}
diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
index 8458ad8..d8b3f1f 100644
--- a/src/PVE/LXC/Setup/Plugin.pm
+++ b/src/PVE/LXC/Setup/Plugin.pm
@@ -48,7 +48,7 @@ sub set_user_password {
}
sub unified_cgroupv2_support {
- my ($self) = @_;
+ my ($self, $conf) = @_;
croak "implement me in sub-class\n";
}
diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanaged.pm
index 3b9febf..e2dd517 100644
--- a/src/PVE/LXC/Setup/Unmanaged.pm
+++ b/src/PVE/LXC/Setup/Unmanaged.pm
@@ -45,7 +45,7 @@ sub set_user_password {
}
sub unified_cgroupv2_support {
- my ($self) = @_;
+ my ($self, $conf) = @_;
return 1; # faking it won't normally hurt ;-)
}
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd
2022-09-09 11:45 [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd Leo Nunner
@ 2022-09-12 9:13 ` Thomas Lamprecht
2022-09-12 9:30 ` Dominik Csapak
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2022-09-12 9:13 UTC (permalink / raw)
To: Proxmox VE development discussion, Leo Nunner
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;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd
2022-09-12 9:13 ` Thomas Lamprecht
@ 2022-09-12 9:30 ` Dominik Csapak
2022-09-12 9:30 ` Dominik Csapak
2022-09-12 9:37 ` Dominik Csapak
0 siblings, 2 replies; 6+ messages in thread
From: Dominik Csapak @ 2022-09-12 9:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Thomas Lamprecht, Leo Nunner
[snip]
>>
>> # 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.
>
AFAICS, not even that is necessary, since a 'LXC::Setup' object has the config in self
so we could do there a '$self->{confg}->{arch}' and omit the parameter passing completely
(or am i missing something else here?)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd
2022-09-12 9:30 ` Dominik Csapak
@ 2022-09-12 9:30 ` Dominik Csapak
2022-09-12 9:43 ` Thomas Lamprecht
2022-09-12 9:37 ` Dominik Csapak
1 sibling, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2022-09-12 9:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Thomas Lamprecht, Leo Nunner
On 9/12/22 11:30, Dominik Csapak wrote:
> [snip]
>>> # 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.
>>
>
> AFAICS, not even that is necessary, since a 'LXC::Setup' object has the config in self
> so we could do there a '$self->{confg}->{arch}' and omit the parameter passing completely
i meant '$self->{conf}->{arch}' ofc
> (or am i missing something else here?)
>
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd
2022-09-12 9:30 ` Dominik Csapak
2022-09-12 9:30 ` Dominik Csapak
@ 2022-09-12 9:37 ` Dominik Csapak
1 sibling, 0 replies; 6+ messages in thread
From: Dominik Csapak @ 2022-09-12 9:37 UTC (permalink / raw)
To: Proxmox VE development discussion, Thomas Lamprecht, Leo Nunner
On 9/12/22 11:30, Dominik Csapak wrote:
> [snip]
>>> # 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.
>>
>
> AFAICS, not even that is necessary, since a 'LXC::Setup' object has the config in self
> so we could do there a '$self->{confg}->{arch}' and omit the parameter passing completely
> (or am i missing something else here?)
>
>
ah ok leo told me that we call it in the 'plugin' where the config is not in self
sorry for the noise ...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd
2022-09-12 9:30 ` Dominik Csapak
@ 2022-09-12 9:43 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2022-09-12 9:43 UTC (permalink / raw)
To: Proxmox VE development discussion, Leo Nunner
On 12/09/2022 11:30, Dominik Csapak wrote:
> On 9/12/22 11:30, Dominik Csapak wrote:
>>>> # 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.
>>>
>>
>> AFAICS, not even that is necessary, since a 'LXC::Setup' object has the config in self
>> so we could do there a '$self->{confg}->{arch}' and omit the parameter passing completely
>
> i meant '$self->{conf}->{arch}' ofc
>
>> (or am i missing something else here?)
This is the plugin though, where $self isn't LXC::Setup but basing off LXC::Setup::Plugin,
and there we don't have any $conf object available, and we explicitly pass $conf to those
methods in most case, which I'd guess is where Leo copied this off from.
Still, I'd like to avoid that pattern for newer adaptions if possible, iow. if only using
up two or three config params at max, as heuristic.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-12 9:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 11:45 [pve-devel] [PATCH container] fix #4192: add new architecture-dependent path to check for newer versions of systemd Leo Nunner
2022-09-12 9:13 ` Thomas Lamprecht
2022-09-12 9:30 ` Dominik Csapak
2022-09-12 9:30 ` Dominik Csapak
2022-09-12 9:43 ` Thomas Lamprecht
2022-09-12 9:37 ` Dominik Csapak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox