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

[Alexandre DERUMIER]

>>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) 

Thanks for review it!


>>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.. 

I need to fix that indeed, I thinked that ifquery output only to stderr if 1 interface 
was in error, but it seem to do it on stdout only.



>>> + 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}, 
>>}; 

ok, thanks, I'll fix that too.






----- Mail original -----
De: "Thomas Lamprecht" <t.lamprecht at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com>
Envoyé: Mardi 4 Juin 2019 06:42:07
Objet: Re: [pve-devel] [PATCH pve-network 2/3] add ifquery compare status

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; 
>