Re: [pve-devel] [PATCH stable-7 manager] pve7to8: Add check for dkms modules

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

Am 23/06/2023 um 15:28 schrieb Christian Ebner:
> ... and warn if at least one is present.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  PVE/CLI/pve7to8.pm | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/PVE/CLI/pve7to8.pm b/PVE/CLI/pve7to8.pm
> index da1e15bc..ecfb4011 100644
> --- a/PVE/CLI/pve7to8.pm
> +++ b/PVE/CLI/pve7to8.pm
> @@ -1287,6 +1287,40 @@ sub check_bootloader {
>      }
>  }
>  
> +sub check_dkms_modules {
> +    log_info("Check if dkms is installed...");
> +
> +    my $path;
> +    my $filter_path = sub {
> +	$path = shift;
> +	chomp $path;

nit: why chomp if only used for a boolean check?

> +    };
> +
> +    run_command(['which', 'dkms'], outfunc => $filter_path, noerr => 1);

please don't use which, it's not standardized and may do weird things,
basically only safe to use if it's a built-in from ones shell.

Maybe just work with the exit code of the run_command execution below?

Otherwise `command -v dkms` would be POSIX
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html

> +
> +    if (!$path) {
> +	log_skip("dkms not installed, no need to check for installed modules");
> +	return;
> +    }
> +
> +    log_info("Check for installed dkms modules...");
> +
> +    my $count;
> +    my $set_count = sub {
> +	$count = scalar @_;
> +    };
> +
> +    eval {
> +	run_command(['dkms', 'status', '-k', '`uname -r`'], outfunc => $set_count, noerr => 1);
> +    };

E.g., maybe it's enough to do:

my $exit_code = eval { run_command(["foo"], outfunc => $set_count, noerr => 1) };

if ($exit_code != 0) {
   # skip
} elsif (!$count) [
 # ...

> +
> +    if (!$count) {
> +	log_pass("no dkms modules found");
> +    } else {
> +	log_warn("dkms modules found, this might cause issues during upgrade.");
> +    }
> +}
> +
>  sub check_misc {
>      print_header("MISCELLANEOUS CHECKS");
>      my $ssh_config = eval { PVE::Tools::file_get_contents('/root/.ssh/config') };
> @@ -1388,6 +1422,7 @@ sub check_misc {
>      check_apt_repos();
>      check_nvidia_vgpu_service();
>      check_bootloader();
> +    check_dkms_modules();
>  }
>  
>  my sub colored_if {