[pve-devel] [PATCH common v2] fix convert_size with decimal numbers and add tests

[Thomas Lamprecht]

Some comments inline

On 11/24/2017 03:24 PM, Dominik Csapak wrote:
> converting from 0.5 gb to mb resulted in 0 mb
> with this patch it correctly returns 512
> 
> also add tests and catch more errors
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * also round when converting down (e.g. gb -> mb)
>   so that we always return an integer
>  src/PVE/Tools.pm          | 17 ++++++++++----
>  test/Makefile             |  1 +
>  test/convert_size_test.pl | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+), 4 deletions(-)
>  create mode 100755 test/convert_size_test.pl
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 13e70f1..c3ac0e8 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1626,9 +1626,11 @@ sub encrypt_pw {
>  }
>  
>  # intended usage: convert_size($val, "kb" => "gb")
> -# on reduction (converting to a bigger unit) we round up by default if
> -# information got lost. E.g. `convert_size(1023, "b" => "kb")` returns 1
> +# we round up to the next integer by default
> +# E.g. `convert_size(1023, "b" => "kb")` returns 1
>  # use $no_round_up to switch this off, above example would then return 0
> +# this is also true for converting down e.g. 0.0005 gb to mb returns 1
> +# (0 if $no_round_up is true)
>  sub convert_size {
>      my ($value, $from, $to, $no_round_up) = @_;
>  
> @@ -1641,21 +1643,28 @@ sub convert_size {
>  	pb => 5,
>      };
>  
> +    $value = $value || 0;

IMO it'd be better check for defined here with //

> +
>      $from = lc($from); $to = lc($to);
>      die "unknown 'from' and/or 'to' units ($from => $to)"
>  	if !(defined($units->{$from}) && defined($units->{$to}));
>  
> +    die "value is not a valid, positive number"

It's maybe worth putting '$value' in this assertion error message, if this
triggers and a user reports the error, we would have much more information
to extrapolate a possible cause from.

> +	if $value !~ m/^[0-9]*\.?[0-9]*$/;
> +
>      my $shift_amount = $units->{$from} - $units->{$to};
>  
>      if ($shift_amount > 0) {
> -	$value <<= ($shift_amount * 10);
> +	$value *=  2**($shift_amount * 10);
> +	my $remainder = $value - int($value);
> +	$value++ if $remainder && !$no_round_up;
>      } elsif ($shift_amount < 0) {
>  	my $remainder = ($value & (1 << abs($shift_amount)*10) - 1);
>  	$value >>= abs($shift_amount) * 10;
>  	$value++ if $remainder && !$no_round_up;

This is still wrong for the test case:
> [ '.5', 'kb', 'gb', undef, 1 ],

As the ceil does not happen here, the elsif would need to be changed to
the same as as the if branch, resulting in a general easier code:



>      }
>  
> -    return $value;
> +    return int($value);
>  }
>  
>  1;
> diff --git a/test/Makefile b/test/Makefile
> index 894093b..b6fe6e0 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -8,6 +8,7 @@ check:
>  	for d in $(SUBDIRS); do $(MAKE) -C $$d check; done
>  	./lock_file.pl
>  	./calendar_event_test.pl
> +	./convert_size_test.pl
>  
>  install: check
>  distclean: clean
> diff --git a/test/convert_size_test.pl b/test/convert_size_test.pl
> new file mode 100755
> index 0000000..3847b23
> --- /dev/null
> +++ b/test/convert_size_test.pl
> @@ -0,0 +1,58 @@
> +#!/usr/bin/perl
> +
> +use lib '../src';
> +use strict;
> +use warnings;
> +use Data::Dumper;
> +use Test::More;
> +
> +use PVE::Tools;
> +
> +my $tests = [
> +    [
> +	1,           # input value
> +	'gb',        # from
> +	'kb',        # to
> +	undef,       # no_round_up
> +	1*1024*1024, # result
> +	undef,       # error string
> +    ],
> +    [ -1, 'gb', 'kb', undef, 1*1024*1024, "value is not a valid, positive number" ],
> +    [ 1.5, 'gb', 'kb', undef, 1.5*1024*1024 ],
> +    [  0.0005, 'gb', 'mb', undef, 1 ],
> +    [  0.0005, 'gb', 'mb', 1, 0 ],
> +    [ '.5', 'gb', 'kb', undef, .5*1024*1024 ],
> +    [ '1.', 'gb', 'kb', undef, 1.*1024*1024 ],
> +    [ '.', 'gb', 'kb', undef, 0 ],
> +    [ '', 'gb', 'kb', undef, 0 ],

Do we really want to allow the above two values?
Silently casting an empty string to 0 seems like not the best idea to me...
The single dot seems  somewhat reasonable. though, while also not too nice

> +    [ '1.1.', 'gb', 'kb', undef, 0, "value is not a valid, positive number" ],
> +    [ 500, 'kb', 'kb', undef, 500, ],
> +    [ 500000, 'b', 'kb', undef, 489, ],
> +    [ 500000, 'b', 'kb', 0, 489, ],
> +    [ 500000, 'b', 'kb', 1, 488, ],
> +    [ 128*1024 - 1, 'b', 'kb', 0, 128, ],
> +    [ 128*1024 - 1, 'b', 'kb', 1, 127, ],
> +    [ "abcdef", 'b', 'kb', 0, 0, "value is not a valid, positive number" ],
> +    [ undef, 'b', 'kb', 0, 0, ],
> +    [ 0, 'b', 'pb', 0, 0, ],
> +    [ 0, 'b', 'yb', 0, 0, "unknown 'from' and/or 'to' units (b => yb)"],
> +    [ 0, 'b', undef, 0, 0, "unknown 'from' and/or 'to' units (b => )"],
> +];
> +
> +foreach my $test (@$tests) {
> +    my ($input, $from, $to, $no_round_up, $expect, $error) = @$test;
> +
> +    my $result = eval { PVE::Tools::convert_size($input, $from, $to, $no_round_up); };
> +    my $err = $@;
> +    if ($error) {
> +	like($err, qr/^\Q$error\E/, "expected error for $input $from -> $to: $error");
> +    } else {
> +	$input = $input // "";
> +	$from = $from // "";
> +	$to = $to // "";
> +	my $round = $no_round_up ? 'floor' : 'ceil';
> +	is($result, $expect, "$input $from converts to $expect $to ($round)");
> +    }
> +};
> +
> +done_testing();
> 

An slightly adapted method could be:

sub convert_size {
    my ($value, $from, $to, $no_round_up) = @_;

    my $units = {
        b  => 0,
        kb => 1,
        mb => 2,
        gb => 3,
        tb => 4,
        pb => 5,
    };

    $value = $value // 0;

    die "both, from and to, must be set" if !$from || !$to;

    $from = lc($from); $to = lc($to);
    die "unknown 'from' and/or 'to' units ($from => $to)"
        if !(defined($units->{$from}) && defined($units->{$to}));

    die "value '$value' is not a valid, positive number"
        if $value eq '' || $value !~ m/^([0-9]+\.?[0-9]*|[0-9]*\.[0-9]+)$/;

    my $shift_amount = ($units->{$from} - $units->{$to}) * 10;

    $value *=  2**$shift_amount;
    $value++ if !$no_round_up && ($value - int($value));

    return int($value);
}

I check if $from and $to are defined, disallow '' and '.' as value and
unified the transformation for $shift_amount < and > 0.
As above stated, not sure how: undef, '', '.' should be handled.
At leas a comment would be nice if we say the get all silently cast to 0...