Re: [PATCH proxmox-datacenter-manager] fix #7187: report: add ethtool output for physical interfaces

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

On 08/05/2026 14:49, Erik Fastermann wrote:
> The implementation mirrors the change in proxmox-backup,
> as the report functionality in pdm is adapted from there.
> 
> Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
> ---
> 
>  NOTE: A similar patch was applied to all products:
>  pmg, pve, pbs, pdm.
> 
>  server/src/report.rs | 63 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/server/src/report.rs b/server/src/report.rs
> index 371d8cf..e327051 100644
> --- a/server/src/report.rs
> +++ b/server/src/report.rs
> @@ -2,6 +2,8 @@ use std::fmt::Write;
>  use std::path::Path;
>  use std::process::Command;
>  
> +use proxmox_network_api::NetworkInterfaceType;
> +
>  // TODO: This was copied from PBS. Might make sense to refactor these a little
>  // bit and move them a `proxmox-system-report` crate or something.
>  
> @@ -46,42 +48,63 @@ fn files() -> Vec<(&'static str, Vec<&'static str>)> {
>      ]
>  }
>  
> -fn commands() -> Vec<(&'static str, Vec<&'static str>)> {
> -    vec![
> -        //  ("<command>", vec![<arg [, arg]>])
> -        ("date", vec!["-R"]),
> +fn commands() -> Vec<(&'static str, Vec<String>)> {
> +    //  ("<command>", vec![<arg [, arg]>])
> +    const BASE_COMMANDS: &[(&str, &[&str])] = &[
> +        ("date", &["-R"]),

if one has to rework basically all of the status quo for a relatively simple
addition it's often a good sign that a different approach might be a better
fit. And if not, the rework should happen upfront in a dedicated commit that
tries to keep the user visible behavior the same, as that makes reviewing and
checking for potential regressions much simpler compared to mixing both.

Here I think it is better to add the new command with dynamic arguments
to a new method, e.g.:

fn dynamic_commands() -> Vec<(&'static str, Vec<String>)> {
    ...

>          (
>              "proxmox-datacenter-manager-admin",
> -            vec!["versions", "--verbose"],
> +            &["versions", "--verbose"],
>          ),
> -        ("proxmox-datacenter-manager-admin", vec!["remote", "list"]),
> +        ("proxmox-datacenter-manager-admin", &["remote", "list"]),
>          (
>              "proxmox-datacenter-manager-admin",
> -            vec!["remote", "subscriptions"],
> +            &["remote", "subscriptions"],
>          ),
>          (
>              "proxmox-datacenter-manager-admin",
> -            vec!["support-status", "get"],
> +            &["support-status", "get"],
>          ),
> -        ("proxmox-boot-tool", vec!["status"]),
> -        ("df", vec!["-h", "-T"]),
> +        ("proxmox-boot-tool", &["status"]),
> +        ("df", &["-h", "-T"]),
>          (
>              "lsblk",
> -            vec![
> +            &[
>                  "--ascii",
>                  "-M",
>                  "-o",
>                  "+HOTPLUG,ROTA,PHY-SEC,FSTYPE,MODEL,TRAN",
>              ],
>          ),
> -        ("ls", vec!["-l", "/dev/disk/by-id", "/dev/disk/by-path"]),
> -        ("zpool", vec!["status"]),
> -        ("zfs", vec!["list"]),
> -        ("zarcstat", vec![]),
> -        ("ip", vec!["-details", "-statistics", "address"]),
> -        ("ip", vec!["-4", "route", "show"]),
> -        ("ip", vec!["-6", "route", "show"]),
> -    ]
> +        ("ls", &["-l", "/dev/disk/by-id", "/dev/disk/by-path"]),
> +        ("zpool", &["status"]),
> +        ("zfs", &["list"]),
> +        ("zarcstat", &[]),
> +        ("ip", &["-details", "-statistics", "address"]),
> +        ("ip", &["-4", "route", "show"]),
> +        ("ip", &["-6", "route", "show"]),
> +    ];
> +
> +    let mut commands: Vec<_> = BASE_COMMANDS
> +        .into_iter()
> +        .map(|(cmd, args)| (*cmd, args.into_iter().map(|arg| arg.to_string()).collect()))
> +        .collect();
> +
> +    match proxmox_network_api::config() {
> +        Ok((config, _)) => {
> +            let ethtool_commands = config
> +                .interfaces
> +                .into_iter()
> +                .filter(|(_, iface)| iface.interface_type == NetworkInterfaceType::Eth)
> +                .map(|(name, _)| ("ethtool", vec![name]));

A simple for loop here would be IMO a bit clearer and should come out also
slightly shorter, i.e. something like:

    for (name, iface) in config.interfaces {
        if iface.interface_type == NetworkInterfaceType::Eth {
            commands.push(("ethtool", vec![name]));
        }
    }

> +            commands.extend(ethtool_commands)
> +        }
> +        Err(err) => {
> +            eprintln!("error while querying network interfaces: {err}")
> +        }
> +    };
> +
> +    commands
>  }
>  
>  // (description, function())
> @@ -137,7 +160,7 @@ fn get_directory_content(path: impl AsRef<Path>) -> String {
>      out
>  }
>  
> -fn get_command_output(exe: &str, args: &Vec<&str>) -> String {
> +fn get_command_output(exe: &str, args: &[String]) -> String {

Can be made generic to accept both:

fn get_command_output<I, S>(exe: &str, args: I) -> String
where
    I: IntoIterator<Item = S>,
    S: AsRef<OsStr>,
{

>      let output = Command::new(exe)
>          .env("PROXMOX_OUTPUT_NO_BORDER", "1")
>          .args(args)