[pve-devel] [PATCH manager] fix #2030: use looks_like_number for number check

[Wolfgang Bumiller]

On Mon, Dec 17, 2018 at 10:58:19AM +0100, Dominik Csapak wrote:
> since numbers can also be in '1.e-10' format, we have to change
> how we check for a number
> 
> Scalar::Util is already core and we use it in PVE::Tools, so
> no new dependecy.
> 
> in case of "NaN" or "Infinity" we omit the key/value pair
> 
> else we quote like before
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/Status/InfluxDB.pm | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
> index 7364e572..27ba4674 100644
> --- a/PVE/Status/InfluxDB.pm
> +++ b/PVE/Status/InfluxDB.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use PVE::Status::Plugin;
>  use Data::Dumper;
>  use PVE::SafeSyslog;
> +use Scalar::Util 'looks_like_number';
>  
>  # example config (/etc/pve/status.cfg)
>  #influxdb:
> @@ -111,8 +112,10 @@ sub build_influxdb_payload {
>  	if (!ref($value) && $value ne '') {
>  	    # value is scalar
>  
> -	    $value = prepare_value($value);
> -	    push @values, "$key=$value";
> +	    my $val = eval { prepare_value($value) };
> +	    if (defined($val)) {
> +		push @values, "$key=$val";
> +	    }
>  	} elsif (ref($value) eq 'HASH') {
>  	    # value is a hash
>  
> @@ -145,8 +148,10 @@ sub get_recursive_values {
>  	if(ref($value) eq 'HASH') {
>  	    push(@values, get_recursive_values($value));
>  	} elsif (!ref($value) && $value ne '') {
> -	    $value = prepare_value($value);
> -	    push @values, "$key=$value";
> +	    my $val = eval { prepare_value($value) };
> +	    if (defined($val)) {
> +		push @values, "$key=$value";
> +	    }
>  	}
>      }
>  
> @@ -156,13 +161,21 @@ sub get_recursive_values {
>  sub prepare_value {
>      my ($value) = @_;
>  
> +    if (looks_like_number($value)) {
> +	if ($value eq 'NaN' || $value =~ /^Inf/) {
> +	    # we cannot send influxdb NaN or Inf
> +	    die;

Wouldn't it be nicer to just `return undef;` here? The eval seems a bit
overkill. Could just use it like:

    if (defined(my $v = prepare_value($value))) {
        push @values, "$key=$v";
    }

> +	}
> +
> +	# influxdb also accepts 1.0e+10, etc.
> +	return $value;
> +    }
> +
>      # if value is not just a number we
>      # have to replace " with \"
>      # and surround it with "
> -    if ($value =~ m/[^\d\.]/) {
> -	$value =~ s/\"/\\\"/g;
> -	$value = "\"$value\"";
> -    }
> +    $value =~ s/\"/\\\"/g;

Since this patch pointed it out - does this make sense? Should we not
also be escaping backslashes?

> +    $value = "\"$value\"";
>  
>      return $value;
>  }
> -- 
> 2.11.0