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

[Friedrich Weber <f.weber@proxmox.com>]

Hi, thanks for tackling this! A few comments inline (note that I only
looked at the PVE patch)

On 08/05/2026 14:39, Erik Fastermann wrote:
> Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
> ---

Please add some rationale to the commit message [1], you can derive it
from the bug #7187 for instance.

> 
>  NOTE: A similar patch was applied to all products:
>  pmg, pve, pbs, pdm.
> 
>  PVE/Report.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/PVE/Report.pm b/PVE/Report.pm
> index 29433d4a..b3a5c08c 100644
> --- a/PVE/Report.pm
> +++ b/PVE/Report.pm
> @@ -3,6 +3,7 @@ package PVE::Report;
>  use strict;
>  use warnings;
>  
> +use PVE::IPRoute2;
>  use PVE::Tools;
>  
>  # output the content of all the files of a directory
> @@ -142,6 +143,14 @@ my $init_report_cmds = sub {
>          },
>      };
>  
> +    my $interfaces = PVE::IPRoute2::ip_link_details();
> +
> +    for my $iface (sort keys %{$interfaces}) {
> +        if (PVE::IPRoute2::ip_link_is_physical($interfaces->{$iface})) {
> +            push @{ $report_def->{network}->{cmds} }, "ethtool $iface";

Everything in $report_def->{network}->{cmds} is essentially passed to
run_command, and passing strings from the outside (like $iface) to
something like run_command can be dangerous (e.g. allow command
injection). Granted, here this is the name of a physical interface, so
the risk of someone injecting something there is low, but as a
precaution I'd still apply a PVE::Tools::shellquote on $iface before
constructing the ethtool command here.

> +        }
> +    }
> +

I'd probably wrap the whole thing in an `eval` with a `if(@)`
afterwards, just to prevent something die'ing in `ip_link_details` or
`ip_link_is_physical` from crashing the report.

>      if (cmd_exists('zfs')) {
>          push @{ $report_def->{volumes}->{cmds} },
>              'zpool status',

[1]
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages