Re: [PATCH storage v2 1/2] nfs: create a helper to run rpcinfo commands

[Fiona Ebner <f.ebner@proxmox.com>]

Am 19.03.26 um 11:42 AM schrieb Maximiliano Sandoval:
> Will be used in the next command as a fallback in the NFSv3 case.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/PVE/Storage/NFSPlugin.pm | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
> index 4cc02c9e..062b3f32 100644
> --- a/src/PVE/Storage/NFSPlugin.pm
> +++ b/src/PVE/Storage/NFSPlugin.pm
> @@ -171,6 +171,20 @@ sub deactivate_storage {
>      }
>  }
>  
> +sub tcp_rpcinfo_cmd {

The helper could be private.

I wouldn't use tcp as a prefix (if using it, rather later in the name).
Other plugins use the plugin name as the prefix for intra-module
helpers. Maybe something like nfs_get_rpcinfo_command?

> +    my ($server, $version) = @_;
> +
> +    my $ip = PVE::JSONSchema::pve_verify_ip($server, 1);
> +    if (!defined($ip)) {
> +        $ip = PVE::Network::get_ip_from_hostname($server);
> +    }
> +
> +    my $transport = PVE::JSONSchema::pve_verify_ipv4($ip, 1) ? 'tcp' : 'tcp6';
> +
> +    # nfsv4 uses a pseudo-filesystem always beginning with / no exports are listed

I feel like putting it on the same line makes it harder to read. There
should be a comma or  dot after the slash. But actually, the comment
does not belong inside the function here, but to the call site. It's the
clarification for why rpcinfo is used rather than listing exports.

> +    return ['/usr/sbin/rpcinfo', '-T', $transport, $ip, 'nfs', $version];
> +}
> +
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
>  
> @@ -181,16 +195,7 @@ sub check_connection {
>  
>      my $is_v4 = defined($opts) && $opts =~ /vers=4.*/;
>      if ($is_v4) {
> -        my $ip = PVE::JSONSchema::pve_verify_ip($server, 1);
> -        if (!defined($ip)) {
> -            $ip = PVE::Network::get_ip_from_hostname($server);
> -        }
> -
> -        my $transport = PVE::JSONSchema::pve_verify_ipv4($ip, 1) ? 'tcp' : 'tcp6';
> -
> -        # nfsv4 uses a pseudo-filesystem always beginning with /
> -        # no exports are listed
> -        $cmd = ['/usr/sbin/rpcinfo', '-T', $transport, $ip, 'nfs', '4'];
> +        $cmd = tcp_rpcinfo_cmd($server, '4');
>      } else {
>          $cmd = ['/sbin/showmount', '--no-headers', '--exports', $server];
>      }