[pve-devel] [PATCH pve-network 2/3] add ifquery compare status

[Thomas Lamprecht]

Dietmar already applied this, but I see a real problem on parsing
(mixing of stdout and stderr outputs, see below) and some style problems/
nitpicks, could you please take a look at them and try to send a follow up?

That'd be great! (FYI: those marked nit have been already fixed up by me)

On 6/3/19 5:57 PM, Alexandre Derumier wrote:
> ---
>  PVE/Network/Network.pm | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/PVE/Network/Network.pm b/PVE/Network/Network.pm
> index ee0b973..3a33ecd 100644
> --- a/PVE/Network/Network.pm
> +++ b/PVE/Network/Network.pm
> @@ -3,6 +3,8 @@ package PVE::Network::Network;
>  use strict;
>  use warnings;
>  use Data::Dumper;
> +use JSON;
> +use PVE::Tools qw(extract_param dir_glob_regex);
>  use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
>  use PVE::Network::Network::Plugin;
>  use PVE::Network::Network::VnetPlugin;
> @@ -62,4 +64,30 @@ sub complete_network {
>      return  $cmdname eq 'add' ? [] : [ PVE::Network::Network::networks_ids($cfg) ];
>  }
>  
> +sub status {
> +
> +    my $cmd = ['ifquery', '-a', '-c', '-o','json'];
> +    my $result;;
                 ^^^
nit: double semi-colon

> +
> +    my $code = sub {
> +        my $line = shift;
> +	$result .= $line;
> +    };

nit: could be also written as:

my $reader = sub { $result .= shift };

> +
> +    eval {
> +	PVE::Tools::run_command($cmd, outfunc => $code, errfunc => $code);
> +    };

nit: if you do most through ifquery et al., so often use run_command
it could be worth it to import above, so that PVE::Tools could
be omitted?

Further, are you sure you want to record stderr too, is it valid JSON?
I mean even if, it will get mixed with stdout output and could get invalid
even if both (the output from stdout and stderr) are valid json..

> +
> +    my $resultjson = JSON::decode_json($result);

above then would throw an exception, if it cannot decode $result
as valid JSON.. btw. "use JSON" exports decode_json by default.

> +    my $interfaces = {};
> +
> +    foreach my $interface (@$resultjson) {

We try to avoid nested hash accesses, so maybe a
my $name = $iface->{name};
$interfaces->{$name}->{status} = $interface->{status};

or even:

$interfaces->{$name} = {
    status => $interface->{status},
    ...,
    config_status => $interface->{config_status},
};

> +	$interfaces->{$interface->{'name'}}->{status} = $interface->{'status'};
> +	$interfaces->{$interface->{'name'}}->{config} = $interface->{'config'};
> +	$interfaces->{$interface->{'name'}}->{config_status} = $interface->{'config_status'};
> +    }
> +
> +    return $interfaces;
> +}
> +
>  1;
>