Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary

[Lukas Wagner <l.wagner@proxmox.com>]

Oh, seems that I have missed v2 - but I think many comments still apply.

On 6/28/23 12:13, Lukas Wagner wrote:
> Hi,
> 
> tested on current `stable-2` branch. Seems to work just fine, at least on a clean PBS installation.
> 
> Some comments regarding the code are inline (nothing major though).
> 
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> 
> On 6/28/23 10:10, Christian Ebner wrote:
>> Adds the pbs2to3 upgrade checker with some basic checks.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   Makefile                             |   1 +
>>   debian/proxmox-backup-server.install |   2 +
>>   src/bin/pbs2to3.rs                   | 602 +++++++++++++++++++++++++++
>>   zsh-completions/_pbs2to3             |  13 +
>>   4 files changed, 618 insertions(+)
>>   create mode 100644 src/bin/pbs2to3.rs
>>   create mode 100644 zsh-completions/_pbs2to3
>>
>> diff --git a/Makefile b/Makefile
>> index b307009d..7477935d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -19,6 +19,7 @@ USR_BIN := \
>>   USR_SBIN := \
>>       proxmox-backup-manager \
>>       proxmox-backup-debug \
>> +    pbs2to3
>>   # Binaries for services:
>>   SERVICE_BIN := \
>> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
>> index 76f50cd0..ebe51aae 100644
>> --- a/debian/proxmox-backup-server.install
>> +++ b/debian/proxmox-backup-server.install
>> @@ -11,6 +11,7 @@ usr/lib/x86_64-linux-gnu/proxmox-backup/proxmox-daily-update
>>   usr/lib/x86_64-linux-gnu/proxmox-backup/sg-tape-cmd
>>   usr/sbin/proxmox-backup-debug
>>   usr/sbin/proxmox-backup-manager
>> +usr/sbin/pbs2to3
>>   usr/bin/pmtx
>>   usr/bin/pmt
>>   usr/bin/proxmox-tape
>> @@ -39,3 +40,4 @@ usr/share/zsh/vendor-completions/_proxmox-backup-manager
>>   usr/share/zsh/vendor-completions/_proxmox-tape
>>   usr/share/zsh/vendor-completions/_pmtx
>>   usr/share/zsh/vendor-completions/_pmt
>> +usr/share/zsh/vendor-completions/_pbs2to3
>> diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs
>> new file mode 100644
>> index 00000000..97d064cd
>> --- /dev/null
>> +++ b/src/bin/pbs2to3.rs
>> @@ -0,0 +1,602 @@
>> +use std::io::Write;
>> +use std::path::Path;
>> +
>> +use anyhow::{format_err, Error};
>> +use regex::Regex;
>> +use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
>> +
>> +use proxmox_apt::repositories::{self, APTRepositoryFile};
>> +use proxmox_backup::api2::node::apt;
>> +
>> +const OLD_SUITE: &str = "bullseye";
>> +const NEW_SUITE: &str = "bookworm";
>> +const PROXMOX_BACKUP_META: &str = "proxmox-backup";
>> +const MIN_PBS_MAJOR: u8 = 2;
>> +const MIN_PBS_MINOR: u8 = 4;
>> +const MIN_PBS_PKGREL: u8 = 1;
>> +
>> +fn main() -> Result<(), Error> {
>> +    let mut checker = Checker::new();
>> +    checker.check_pbs_packages()?;
>> +    checker.check_misc()?;
>> +    checker.summary()?;
>> +    Ok(())
>> +}
>> +
>> +struct Checker {
>> +    output: ConsoleOutput,
>> +    upgraded: bool,
>> +}
>> +
>> +impl Checker {
>> +    pub fn new() -> Self {
>> +        Self {
>> +            output: ConsoleOutput::new(),
>> +            upgraded: false,
>> +        }
>> +    }
>> +
> 
> The `check_pbs_packages` function is a bit long and hard to read, maybe break this
> into multiple subroutines? Generally I prefer to have functions that fit entirely on screen without
> scrolling, unless there is a good reason against it.
>> +    pub fn check_pbs_packages(&mut self) -> Result<(), Error> {
>> +        self.output.print_header("CHECKING VERSION INFORMATION FOR PBS PACKAGES")?;
>> +        self.output.log_info("Checking for package updates..")?;
>> +
>> +        let result = Self::get_upgradable_packages();
>> +        match result {
>> +            Err(err) => {
>> +                self.output.log_warn(format!("{}", err).as_str())?;
>                                                           ^
> Nit: You could inline the variable here: `format!("{err}")`
> Also applies to many other locations in the code, I'll refrain from highlighting the other occurences ;).
> 
> 
>> +                self.output.log_fail("unable to retrieve list of package updates!")?;
>> +            }
>> +            Ok(cache) => {
>> +                if cache.package_status.is_empty() {
>> +                    self.output.log_pass("all packages up-to-date")?;
>> +                } else {
>> +                    let pkgs = cache.package_status.iter()
>> +                        .map(|pkg| format!("{}, ", pkg.package.clone()))
>> +                        .collect::<String>()> +                    self.output.log_warn(
>> +                        format!(
>> +                            "updates for the following packages are available:\
>> +                            \n      {}",
>> +                            &pkgs[..pkgs.len()-2]
>> +                        ).as_str()
> You could change the signatures for the `log_*` functions to
> 
>      pub fn log_pass<T: AsRef<str>>(&mut self, message: T) -> Result<(), Error> {
>          self.log_line(LogLevel::Pass, message.as_ref())
>      }
> 
> -> then the function can take a `String` as a well as `&str`, allowing you to omit the
> `as_str()` calls in various places.
> 
>> +                    )?;
>> +                }
>> +            }
>> +        }
>> +
>> +        self.output.log_info("Checking proxmox backup server package version..")?;
>> +        let pkg_versions = apt::get_versions()?;
>> +
>> +        let pbs_meta_pkg = pkg_versions.iter().find(|pkg| {
>> +            pkg.package.as_str() == PROXMOX_BACKUP_META
>> +        });
>> +
>> +        if let Some(pbs_meta_pkg) = pbs_meta_pkg {
>> +            let pkg_version = Regex::new(r"^(\d+)\.(\d+)[.-](\d+)")?;
>> +            let captures = pkg_version.captures(&pbs_meta_pkg.old_version);
>> +            if let Some(captures) = captures {
>> +                let maj = Self::extract_version_from_captures(1, &captures)?;
>> +                let min = Self::extract_version_from_captures(2, &captures)?;
>> +                let pkgrel = Self::extract_version_from_captures(3, &captures)?;
>> +
>> +                if maj > MIN_PBS_MAJOR {
>> +                    self.output.log_pass(
>> +                        format!("Already upgraded to Proxmox Backup Server {}", maj).as_str()
>> +                    )?;
>> +                    self.upgraded = true;
>> +                } else if maj >= MIN_PBS_MAJOR && min >= MIN_PBS_MINOR && pkgrel >= MIN_PBS_PKGREL {
>> +                    self.output.log_pass(
>> +                        format!(
>> +                            "'{}' has version >= {}.{}-{}",
>> +                            PROXMOX_BACKUP_META,
>> +                            MIN_PBS_MAJOR,
>> +                            MIN_PBS_MINOR,
>> +                            MIN_PBS_PKGREL,
>> +                        ).as_str()
>> +                    )?;
>> +                } else {
>> +                    self.output.log_fail(
>> +                        format!(
>> +                            "'{}' package is too old, please upgrade to >= {}.{}-{}",
>> +                            PROXMOX_BACKUP_META,
>> +                            MIN_PBS_MAJOR,
>> +                            MIN_PBS_MINOR,
>> +                            MIN_PBS_PKGREL,
>> +                        ).as_str()
>> +                    )?;
>> +                }
>> +            } else {
>> +                self.output.log_fail(
>> +                    format!(
>> +                        "could not match the '{}' package version, is it installed?",
>> +                        PROXMOX_BACKUP_META,
>> +                    ).as_str()
>> +                )?;
>> +            }
>> +
>> +            self.output.log_info("Check running kernel version..")?;
>> +            let mut krunning = Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?;
>> +            let mut kinstalled = "pve-kernel-6.2";
>> +            if !self.upgraded {
>> +                krunning = Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?;
>> +                kinstalled = "pve-kernel-5.15";
>> +            }
> 
> Nit: I'd probably write something like:
> 
> let (krunning, kinstalled) = if self.upgraded {
>      (
>        Regex::new(...),
>        "pve-kernel-..."
>      )
> } else {
>     ...
> }
> 
> That way you can avoid the `mut` and also avoid compiling a regex that is not actually used.
> 
>> +
>> +            let mut command = std::process::Command::new("uname");
>> +            command.arg("-r");
>> +            match command.output() {
> 
> Nit: Also here you could avoid the `mut` and use the `Command` in a chained fashion:
> 
> let output = Command::new(..)
>      .arg(...)
>      .output();
> 
> match output {
>      ...
> }
> 
>> +                Err(_err) => self.output.log_fail("unable to determine running kernel version.")?,
>> +                Ok(ret) => {
>> +                    let running_version = std::str::from_utf8(&ret.stdout[..ret.stdout.len()-1])?;
>> +                    if krunning.is_match(running_version) {
>> +                        if self.upgraded {
>> +                            self.output.log_pass(
>> +                                format!(
>> +                                    "running new kernel '{}' after upgrade.",
>> +                                    running_version,
>> +                                ).as_str()
>> +                            )?;
>> +                        } else {
>> +                            self.output.log_pass(
>> +                                format!(
>> +                                    "running kernel '{}' is considered suitable for upgrade.",
>> +                                    running_version,
>> +                                ).as_str()
>> +                            )?;
>> +                        }
>> +                    } else {
>> +                        let installed_kernel = pkg_versions.iter().find(|pkg| {
>> +                            pkg.package.as_str() == kinstalled
>> +                        });
>> +                        if installed_kernel.is_some() {
>> +                            self.output.log_warn(
>> +                                format!(
>> +                                    "a suitable kernel '{}' is installed, but an unsuitable '{}' \
>> +                                    is booted, missing reboot?!",
>> +                                    kinstalled,
>> +                                    running_version,
>> +                                ).as_str()
>> +                            )?;
>> +                        } else {
>> +                            self.output.log_warn(
>> +                                format!(
>> +                                    "unexpected running and installed kernel '{}'.",
>> +                                    running_version,
>> +                                ).as_str()
>> +                            )?;
>> +                        }
>> +                    }
>> +                }
>> +            }
>> +
>> +        } else {
>> +            self.output.log_fail(format!("'{}' package not found!", PROXMOX_BACKUP_META).as_str())?;
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>> +    fn extract_version_from_captures(index: usize, captures: &regex::Captures) -> Result<u8, Error> {
>> +        if let Some(capture) = captures.get(index) {
>> +            let val = capture.as_str().parse::<u8>()?;
>> +            Ok(val)
>> +        } else {
>> +            Ok(0)
>> +        }
>> +    }
>> +
>> +    fn check_bootloader(&mut self) -> Result<(), Error> {
>> +        self.output.log_info("Checking bootloader configuration...")?;
>> +
>> +        // PBS packages version check needs to be run before
>> +        if !self.upgraded {
>> +            self.output.log_skip("not yet upgraded, no need to check the presence of systemd-boot")?;
>> +        }
>> +
>> +        if !Path::is_file(Path::new("/etc/kernel/proxmox-boot-uuids")) {
>                       ^
> `Path::is_file(&self)` takes `&self` as parameter, so I'd just write:
> 
> `if !Path::new("...").is_file() {`
> 
>> +            self.output.log_skip("proxmox-boot-tool not used for bootloader configuration")?;
>> +            return Ok(());
>> +        }
>> +
>> +        if !Path::is_dir(Path::new("/sys/firmware/efi")) {
> Here as well.
>> +            self.output.log_skip("System booted in legacy-mode - no need for systemd-boot")?;
>> +            return Ok(());
>> +        }/
>> +
>> +        if Path::is_file(Path::new("/usr/share/doc/systemd-boot/changelog.Debian.gz")) {
> Here as well.
>> +            self.output.log_pass("systemd-boot is installed")?;
>> +        } else {
>> +            self.output.log_warn(
>> +                "proxmox-boot-tool is used for bootloader configuration in uefi mode \
>> +                 but the separate systemd-boot package, existing in Debian Bookworm \
>> +                 is not installed.\n\
>> +                 initializing new ESPs will not work unitl the package is installed."
>> +            )?;
>> +        }
>> +        Ok(())
>> +    }
>> +
>> +    fn check_apt_repos(&mut self) -> Result<(), Error> {
>> +        self.output.log_info("Checking for package repository suite mismatches..")?;
>> +
>> +        let mut strange = false;
>> +        let mut mismatches = Vec::new();
>> +
>> +        let (repo_files, _repo_errors, _digest) = repositories::repositories()?;
>> +        for repo_file in repo_files {
>> +            let (found_strange, mut found_mismatches) = self.check_repo_file(repo_file)?;
>> +            if found_strange {
>> +                strange = true;
>> +            }
>> +            mismatches.append(&mut found_mismatches);
>> +        }
>> +
>> +        match (mismatches.is_empty(), strange) {
>> +            (true, false) => self.output.log_pass("found no suite mismatch")?,
>> +            (true, true) => self.output.log_notice(
>> +                "found no suite mismatches, but found at least one strange suite"
>> +            )?,
>> +            (false, _) => {
>> +                let mut message = String::from(
>> +                    "Found mixed old and new packages repository suites, fix before upgrading!\
>> +                    \n      Mismatches:"
>> +                );
>> +                for (suite, location) in mismatches.iter() {
>> +                    message.push_str(
>> +                        format!("\n      found suite '{}' at '{}'", suite, location).as_str()
>> +                    );
>> +                }
>> +                message.push('\n');
>> +                self.output.log_fail(message.as_str())?
>> +            }
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>> +    pub fn check_misc(&mut self) -> Result<(), Error> {
>> +        self.output.print_header("MISCELLANEOUS CHECKS")?;
>> +        self.check_pbs_services()?;
>> +        self.check_time_sync()?;
>> +        self.check_apt_repos()?;
>> +        self.check_bootloader()?;
>> +        Ok(())
>> +    }
>> +
>> +    pub fn summary(&mut self) -> Result<(), Error> {
>> +        self.output.print_summary()
>> +    }
>> +
> 
> The function below is deeply nested (up to 7 levels, if I counted correctly), maybe break
> stuff out into smaller functions?
>> +    fn check_repo_file(
>> +        &mut self,
>> +        repo_file: APTRepositoryFile
>> +    ) -> Result<(bool, Vec<(String, String)>), Error> {
>> +        let deb_suite = Regex::new(r"(\w+)(-updates|-backports|-security)?$")?;
>> +        let mut strange_suite = false;
>> +        let mut found_suite: Option<(String, String)> = None;
>> +        let mut mismatches = Vec::new();
>> +
>> +        for repo in repo_file.repositories {
>> +            for suite in &repo.suites {
>> +                if let Some(suite) = deb_suite.captures(suite) {
>> +                    if let Some(suite_match) = suite.get(1) {
>> +                        let suite = suite_match.as_str();
>> +                        let location = repo_file.path.clone().unwrap_or(String::new());
>                                                                                ^
> clippy: You can use `unwrap_or_default` here.
> 
>> +
>> +                        if suite != OLD_SUITE && suite != NEW_SUITE {
>> +                            self.output.log_notice(format!(
>> +                                "found unusual suite '{}', neighter old '{}' nor new '{}'..\
>> +                                 \n        Affected file {}\
>> +                                 \n        Please assure this is shipping compatible packages for \
>> +                                 the upgrade!",
>> +                                 suite,
>> +                                 OLD_SUITE,
>> +                                 NEW_SUITE,
>> +                                 location,
>> +                            ).as_str())?;
>> +                            strange_suite = true;
>> +                            continue;
>> +                        }
>> +
>> +                        if let Some((ref current_suite, ref current_location)) = found_suite {
>> +                            if suite != current_suite {
>> +                                if mismatches.is_empty() {
>> +                                    mismatches.push(
>> +                                        (current_suite.clone(), current_location.clone())
>> +                                    );
>> +                                    mismatches.push((suite.to_string(), location));
>> +                                } else {
>> +                                    mismatches.push((suite.to_string(), location));
>> +                                }
>> +                            }
>> +                        } else {
>> +                            found_suite = Some((suite.to_string(), location));
>> +                        }
>> +                    }
>> +                }
>> +            }
>> +        }
>> +
>> +        Ok((strange_suite, mismatches))
>> +    }
>> +
>> +    fn get_systemd_unit_state(
>> +        &mut self,
>> +        unit: &str,
>> +    ) -> Result<(SystemdUnitState, SystemdUnitState), Error> {
>> +        let mut command = std::process::Command::new("systemctl");
>> +        command.arg("is-enabled");
>> +        command.arg(unit);
>> +        let output = command.output().map_err(|err| {
>> +            format_err!("failed to execute {:?} - {}", command, err)
>> +        })?;
>> +
>> +        let is_enabled_state = match output.stdout.as_slice() {
>> +            b"enabled\n" => SystemdUnitState::Enabled,
>> +            b"disabled\n" => SystemdUnitState::Disabled,
>> +            _ => SystemdUnitState::Unknown,
>> +        };
>> +
>> +        let mut command = std::process::Command::new("systemctl");
>> +        command.arg("is-active");
>> +        command.arg(unit);
>> +        let output = command.output().map_err(|err| {
>> +            format_err!("failed to execute {:?} - {}", command, err)
>> +        })?;
>> +
>> +        let is_active_state = match output.stdout.as_slice() {
>> +            b"active\n" => SystemdUnitState::Active,
>> +            b"inactive\n" => SystemdUnitState::Inactive,
>> +            b"failed\n" => SystemdUnitState::Failed,
>> +            _ => SystemdUnitState::Unknown,
>> +        };
>> +        Ok((is_enabled_state, is_active_state))
>> +    }
>> +
>> +    fn check_pbs_services(&mut self) -> Result<(), Error> {
>> +        self.output.log_info("Checking pbs daemon services..")?;
>> +
>> +        for service in ["proxmox-backup.service", "proxmox-backup-proxy.service"] {
>> +            match self.get_systemd_unit_state(service)? {
>> +                (_, SystemdUnitState::Active) => {
>> +                    self.output.log_pass(
>> +                        format!("systemd unit '{}' is in state 'active'", service).as_str()
>> +                    )?;
>> +                }
>> +                (_, SystemdUnitState::Inactive) => {
>> +                    self.output.log_fail(
>> +                        format!(
>> +                            "systemd unit '{}' is in state 'inactive'\
>> +                            \n    Please check the service for errors and start it.",
>> +                            service,
>> +                        ).as_str()
>> +                    )?;
>> +                }
>> +                (_, SystemdUnitState::Failed) => {
>> +                    self.output.log_fail(
>> +                        format!(
>> +                            "systemd unit '{}' is in state 'failed'\
>> +                            \n    Please check the service for errors and start it.",
>> +                            service,
>> +                        ).as_str()
>> +                    )?;
>> +                }
>> +                (_, _) => {
>> +                    self.output.log_fail(
>> +                        format!(
>> +                            "systemd unit '{}' is not in state 'active'\
>> +                            \n    Please check the service for errors and start it.",
>> +                            service,
>> +                        ).as_str()
>> +                    )?;
>> +                }
>> +            }
>> +        }
>> +        Ok(())
>> +    }
>> +
>> +    fn check_time_sync(&mut self) -> Result<(), Error> {
>> +        self.output.log_info("Checking for supported & active NTP service..")?;
>> +        if self.get_systemd_unit_state("systemd-timesyncd.service")?.1 == SystemdUnitState::Active {
>> +            self.output.log_warn(
>> +                "systemd-timesyncd is not the best choice for time-keeping on servers, due to only \
>> +                applying updates on boot.\
>> +                \n       While not necessary for the upgrade it's recommended to use one of:\
>> +                \n        * chrony (Default in new Proxmox VE installations)\
>> +                \n        * ntpsec\
>> +                \n        * openntpd"
>> +            )?;
>> +        } else if self.get_systemd_unit_state("ntp.service")?.1 == SystemdUnitState::Active {
>> +            self.output.log_info(
>> +                "Debian deprecated and removed the ntp package for Bookworm, but the system \
>> +                will automatically migrate to the 'ntpsec' replacement package on upgrade."
>> +            )?;
>> +        } else if self.get_systemd_unit_state("chrony.service")?.1 == SystemdUnitState::Active ||
>> +            self.get_systemd_unit_state("openntpd.service")?.1 == SystemdUnitState::Active ||
>> +            self.get_systemd_unit_state("ntpsec.service")?.1 == SystemdUnitState::Active
>> +        {
>> +            self.output.log_pass("Detected active time synchronisation unit")?;
>> +        } else {
>> +            self.output.log_warn(
>> +                "No (active) time synchronisation daemon (NTP) detected, but synchronized systems \
>> +                are important!"
>> +            )?;
>> +        }
>> +        Ok(())
>> +    }
>> +
>> +    fn get_upgradable_packages() -> Result<proxmox_backup::tools::apt::PkgState, Error> {
>> +        let cache = if let Ok(false) = proxmox_backup::tools::apt::pkg_cache_expired() {
>> +            if let Ok(Some(cache)) = proxmox_backup::tools::apt::read_pkg_state() {
>> +                cache
>> +            } else {
>> +                proxmox_backup::tools::apt::update_cache()?
>> +            }
>> +        } else {
>> +            proxmox_backup::tools::apt::update_cache()?
>> +        };
>> +
>> +        Ok(cache)
>> +    }
>> +}
>> +
>> +#[derive(PartialEq)]
>> +enum SystemdUnitState {
>> +    Active,
>> +    Enabled,
>> +    Disabled,
>> +    Failed,
>> +    Inactive,
>> +    Unknown,
>> +}
>> +
>> +#[derive(Default)]
>> +struct Counters {
>> +    pass: u64,
>> +    skip: u64,
>> +    notice: u64,
>> +    warn: u64,
>> +    fail: u64,
>> +}
>> +
>> +enum LogLevel {
>> +    Pass,
>> +    Info,
>> +    Skip,
>> +    Notice,
>> +    Warn,
>> +    Fail,
>> +}
>> +
>> +struct ConsoleOutput {
>> +    stream: StandardStream,
>> +    first_header: bool,
>> +    counters: Counters,
>> +}
>> +
>> +impl ConsoleOutput {
>> +    pub fn new() -> Self {
>> +        Self {
>> +            stream: StandardStream::stdout(ColorChoice::Always),
>> +            first_header: true,
>> +            counters: Counters::default(),
>> +        }
>> +    }
>> +
>> +    pub fn print_header(&mut self, message: &str) -> Result<(), Error> {
>> +        if !self.first_header {
>> +            writeln!(&mut self.stream, "")?;
>                                                 ^
> clippy: You can remove the empty string if you just want to write a new line.
> 
>> +        }
>> +        self.first_header = false;
>> +        writeln!(&mut self.stream, "= {message} =\n")?;
>> +        Ok(())
>> +    }
>> +
>> +    pub fn set_color(&mut self, color: Color) -> Result<(), Error> {
>> +        self.stream.set_color(ColorSpec::new().set_fg(Some(color)))?;
>> +        Ok(())
>> +    }
>> +
>> +    pub fn log_line(&mut self, level: LogLevel, message: &str) -> Result<(), Error> {
>> +        match level {
>> +            LogLevel::Pass => {
>> +                self.counters.pass += 1;
>> +                self.set_color(Color::Green)?;
>> +                writeln!(&mut self.stream, "PASS: {}", message)?;
>> +                self.set_color(Color::White)?;
>> +            }
>> +            LogLevel::Info => {
>> +                writeln!(&mut self.stream, "INFO: {}", message)?;
>> +            }
>> +            LogLevel::Skip => {
>> +                self.counters.skip += 1;
>> +                writeln!(&mut self.stream, "SKIP: {}", message)?;
>> +            }
>> +            LogLevel::Notice => {
>> +                self.counters.notice += 1;
>> +                writeln!(&mut self.stream, "NOTICE: {}", message)?;
>> +            }
>> +            LogLevel::Warn=> {
>> +                self.counters.warn += 1;
>> +                self.set_color(Color::Yellow)?;
>> +                writeln!(&mut self.stream, "WARN: {}", message)?;
>> +                self.set_color(Color::White)?;
>> +            }
>> +            LogLevel::Fail => {
>> +                self.counters.fail += 1;
>> +                self.set_color(Color::Red)?;
>> +                writeln!(&mut self.stream, "FAIL: {}", message)?;
>> +                self.set_color(Color::White)?;
>> +            }
>> +        }
>> +        Ok(())
>> +    }
>> +
>> +    pub fn log_pass(&mut self, message: &str) -> Result<(), Error> {
>> +        self.log_line(LogLevel::Pass, message)
>> +    }
>> +
>> +    pub fn log_info(&mut self, message: &str) -> Result<(), Error> {
>> +        self.log_line(LogLevel::Info, message)
>> +    }
>> +
>> +    pub fn log_skip(&mut self, message: &str) -> Result<(), Error> {
>> +        self.log_line(LogLevel::Skip, message)
>> +    }
>> +
>> +    pub fn log_notice(&mut self, message: &str) -> Result<(), Error> {
>> +        self.log_line(LogLevel::Notice, message)
>> +    }
>> +
>> +    pub fn log_warn(&mut self, message: &str) -> Result<(), Error> {
>> +        self.log_line(LogLevel::Warn, message)
>> +    }
>> +
>> +    pub fn log_fail(&mut self, message: &str) -> Result<(), Error> {
>> +        self.log_line(LogLevel::Fail, message)
>> +    }
>> +
>> +    pub fn print_summary(&mut self) -> Result<(), Error> {
>> +        self.print_header("SUMMARY")?;
>> +
>> +        let total = self.counters.fail + self.counters.pass + self.counters.notice +
>> +            self.counters.skip + self.counters.warn;
>> +
>> +        self.set_color(Color::White)?;
>> +        writeln!(&mut self.stream, "TOTAL:     {total}")?;
>> +        self.set_color(Color::Green)?;
>> +        writeln!(&mut self.stream, "PASSED:    {}", self.counters.pass)?;
>> +        self.set_color(Color::White)?;
>> +        writeln!(&mut self.stream, "SKIPPED:   {}", self.counters.skip)?;
>> +        writeln!(&mut self.stream, "NOTICE:    {}", self.counters.notice)?;
>> +        if self.counters.warn > 0 {
>> +            self.set_color(Color::Yellow)?;
>> +            writeln!(&mut self.stream, "WARNINGS:  {}", self.counters.warn)?;
>> +        }
>> +        if self.counters.fail > 0 {
>> +            self.set_color(Color::Red)?;
>> +            writeln!(&mut self.stream, "FAILURES:  {}", self.counters.fail)?;
>> +        }
>> +        if self.counters.warn > 0 || self.counters.fail > 0 {
>> +            let mut color = Color::Yellow;
>> +            if self.counters.fail > 0 {
>> +                color = Color::Red;
>> +            }
>> +
>> +            self.set_color(color)?;
>> +            writeln!(
>> +                &mut self.stream,
>> +                "\nATTENTION: Please check the output for detailed information!",
>> +            )?;
>> +            if self.counters.fail > 0 {
>> +                writeln!(
>> +                    &mut self.stream,
>> +                    "Try to solve the problems one at a time and rerun this checklist tool again.",
>> +                )?;
>> +            }
>> +        }
>> +        self.set_color(Color::White)?;
>> +        Ok(())
>> +    }
>> +}
>> +
>> diff --git a/zsh-completions/_pbs2to3 b/zsh-completions/_pbs2to3
>> new file mode 100644
>> index 00000000..f6dc3d67
>> --- /dev/null
>> +++ b/zsh-completions/_pbs2to3
>> @@ -0,0 +1,13 @@
>> +#compdef _pbs2to3() pbs2to3
>> +
>> +function _pbs2to3() {
>> +    local cwords line point cmd curr prev
>> +    cwords=${#words[@]}
>> +    line=$words
>> +    point=${#line}
>> +    cmd=${words[1]}
>> +    curr=${words[cwords]}
>> +    prev=${words[cwords-1]}
>> +    compadd -- $(COMP_CWORD="$cwords" COMP_LINE="$line" COMP_POINT="$point" \
>> +        pbs2to3 bashcomplete "$cmd" "$curr" "$prev")
>> +}
> 

-- 
- Lukas