From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 626951FF13A for ; Wed, 13 May 2026 14:19:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 629F2BC2C; Wed, 13 May 2026 14:19:36 +0200 (CEST) Message-ID: <9a7d3a53-9b05-4ddb-94af-662ff2ffff1b@proxmox.com> Date: Wed, 13 May 2026 14:19:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH proxmox-datacenter-manager] fix #7187: report: add ethtool output for physical interfaces To: Erik Fastermann , pdm-devel@lists.proxmox.com References: <20260508125054.62224-1-e.fastermann@proxmox.com> Content-Language: en-US, de-DE From: Thomas Lamprecht In-Reply-To: <20260508125054.62224-1-e.fastermann@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778674763896 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.003 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: WUW2Q7DLH7BIFUECBNOWVACHLISFQXZ2 X-Message-ID-Hash: WUW2Q7DLH7BIFUECBNOWVACHLISFQXZ2 X-MailFrom: t.lamprecht@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > > 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![ > - // ("", vec![]) > - ("date", vec!["-R"]), > +fn commands() -> Vec<(&'static str, Vec)> { > + // ("", vec![]) > + 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)> { ... > ( > "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) -> 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(exe: &str, args: I) -> String where I: IntoIterator, S: AsRef, { > let output = Command::new(exe) > .env("PROXMOX_OUTPUT_NO_BORDER", "1") > .args(args)