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

[Dominik Csapak]

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;
+
     $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);
+	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;
     }
 
-    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 ],
+    [ '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();
-- 
2.11.0