* Re: [PATCH proxmox-datacenter-manager] fix #7187: report: add ethtool output for physical interfaces
2026-05-08 12:50 [PATCH proxmox-datacenter-manager] fix #7187: report: add ethtool output for physical interfaces Erik Fastermann
@ 2026-05-13 12:19 ` Thomas Lamprecht
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2026-05-13 12:19 UTC (permalink / raw)
To: Erik Fastermann, pdm-devel
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)
^ permalink raw reply [flat|nested] 2+ messages in thread