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

[Fabian Grünbichler]

On Fri, Nov 24, 2017 at 02:33:47PM +0100, 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

for completeness sake and to prevent future problems, maybe we can add
an explicit comment stating that this helper ALWAYS returns an int no
matter in which direction the conversion goes - and then actually make
it so also for weird corner cases like (1.000000000002, 'gb' => 'mb') ;)

> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  src/PVE/Tools.pm          |  7 +++++-
>  test/Makefile             |  1 +
>  test/convert_size_test.pl | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>  create mode 100755 test/convert_size_test.pl
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 13e70f1..f32db80 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1641,14 +1641,19 @@ sub convert_size {
>  	pb => 5,
>      };
>  
> +    $value = $value || 0;
> +
>      $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"
> +	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);
>      } elsif ($shift_amount < 0) {
>  	my $remainder = ($value & (1 << abs($shift_amount)*10) - 1);
>  	$value >>= abs($shift_amount) * 10;
> 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..252c6ae
> --- /dev/null
> +++ b/test/convert_size_test.pl
> @@ -0,0 +1,56 @@
> +#!/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,],
> +    [ '.5', 'gb', 'kb', undef, .5*1024*1024,],
> +    [ '1.', 'gb', 'kb', undef, 1.*1024*1024,],
> +    [ '.', 'gb', 'kb', undef, 0*1024*1024,],
> +    [ '', 'gb', 'kb', undef, 0*1024*1024,],
> +    [ '1.1.', 'gb', 'kb', undef, .5*1024*1024, "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();
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel