Rework unit parsing

Due to how conversions were implemented previously, 1TB failed to
parse on 32-bit, as we were overflowing during computation, even
though the final result would fit easily.

This patch moves the parsing of the scaling factor to a separate
function, and all the conversions are done via the Rational type
(which has unlimited arbitrary precision), and conversion to the
desired type only happens at the last step.

The unit-tests are adjusted too, unfortunately they use the same
algorithm as the code… suggestions on how to improve things are
welcome.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

diff --git a/htools/Ganeti/HTools/QC.hs b/htools/Ganeti/HTools/QC.hs
index 22a5516..fb9d7e3 100644 (file)
--- a/htools/Ganeti/HTools/QC.hs
+++ b/htools/Ganeti/HTools/QC.hs
@@ -537,17 +537,20 @@ prop_Utils_select_undefv lst1 (NonEmpty lst2) =
           cndlist = flist ++ tlist ++ [undefined]
 
 prop_Utils_parseUnit (NonNegative n) =
-  Utils.parseUnit (show n) == Types.Ok n &&
-  Utils.parseUnit (show n ++ "m") == Types.Ok n &&
-  (case Utils.parseUnit (show n ++ "M") of
-     Types.Ok m -> if n > 0
-                     then m < n  -- for positive values, X MB is < than X MiB
-                     else m == 0 -- but for 0, 0 MB == 0 MiB
-     Types.Bad _ -> False) &&
-  Utils.parseUnit (show n ++ "g") == Types.Ok (n*1024) &&
-  Utils.parseUnit (show n ++ "t") == Types.Ok (n*1048576) &&
-  Types.isBad (Utils.parseUnit (show n ++ "x")::Types.Result Int)
-    where _types = n::Int
+  Utils.parseUnit (show n) ==? Types.Ok n .&&.
+  Utils.parseUnit (show n ++ "m") ==? Types.Ok n .&&.
+  Utils.parseUnit (show n ++ "M") ==? Types.Ok (truncate n_mb::Int) .&&.
+  Utils.parseUnit (show n ++ "g") ==? Types.Ok (n*1024) .&&.
+  Utils.parseUnit (show n ++ "G") ==? Types.Ok (truncate n_gb::Int) .&&.
+  Utils.parseUnit (show n ++ "t") ==? Types.Ok (n*1048576) .&&.
+  Utils.parseUnit (show n ++ "T") ==? Types.Ok (truncate n_tb::Int) .&&.
+  printTestCase "Internal error/overflow?"
+    (n_mb >=0 && n_gb >= 0 && n_tb >= 0) .&&.
+  property (Types.isBad (Utils.parseUnit (show n ++ "x")::Types.Result Int))
+  where _types = (n::Int)
+        n_mb = (fromIntegral n::Rational) * 1000 * 1000 / 1024 / 1024
+        n_gb = n_mb * 1000
+        n_tb = n_gb * 1000
 
 -- | Test list for the Utils module.
 testSuite "Utils"

diff --git a/htools/Ganeti/HTools/Utils.hs b/htools/Ganeti/HTools/Utils.hs
index f3e3e3d..2bc7360 100644 (file)
--- a/htools/Ganeti/HTools/Utils.hs
+++ b/htools/Ganeti/HTools/Utils.hs
@@ -40,6 +40,7 @@ module Ganeti.HTools.Utils
 
 import Data.Char (toUpper)
 import Data.List
+import Data.Ratio ((%))
 
 import Debug.Trace
 
@@ -163,6 +164,25 @@ printTable lp header rows isnum =
   unlines . map ((++) lp) . map ((:) ' ' . unwords) $
   formatTable (header:rows) isnum
 
+-- | Converts a unit (e.g. m or GB) into a scaling factor.
+parseUnitValue :: (Monad m) => String -> m Rational
+parseUnitValue unit
+  -- binary conversions first
+  | null unit                     = return 1
+  | unit == "m" || upper == "MIB" = return 1
+  | unit == "g" || upper == "GIB" = return kbBinary
+  | unit == "t" || upper == "TIB" = return $ kbBinary * kbBinary
+  -- SI conversions
+  | unit == "M" || upper == "MB"  = return mbFactor
+  | unit == "G" || upper == "GB"  = return $ mbFactor * kbDecimal
+  | unit == "T" || upper == "TB"  = return $ mbFactor * kbDecimal * kbDecimal
+  | otherwise = fail $ "Unknown unit '" ++ unit ++ "'"
+  where upper = map toUpper unit
+        kbBinary = 1024
+        kbDecimal = 1000
+        decToBin = kbDecimal / kbBinary -- factor for 1K conversion
+        mbFactor = decToBin * decToBin -- twice the factor for just 1K
+
 -- | Tries to extract number and scale from the given string.
 --
 -- Input must be in the format NUMBER+ SPACE* [UNIT]. If no unit is
@@ -172,20 +192,10 @@ parseUnit :: (Monad m, Integral a, Read a) => String -> m a
 parseUnit str =
   -- TODO: enhance this by splitting the unit parsing code out and
   -- accepting floating-point numbers
-  case reads str of
+  case (reads str::[(Int, String)]) of
     [(v, suffix)] ->
       let unit = dropWhile (== ' ') suffix
-          upper = map toUpper unit
-          siConvert x = x * 1000000 `div` 1048576
-      in case () of
-           _ | null unit -> return v
-             | unit == "m" || upper == "MIB" -> return v
-             | unit == "M" || upper == "MB"  -> return $ siConvert v
-             | unit == "g" || upper == "GIB" -> return $ v * 1024
-             | unit == "G" || upper == "GB"  -> return $ siConvert
-                                                (v * 1000)
-             | unit == "t" || upper == "TIB" -> return $ v * 1048576
-             | unit == "T" || upper == "TB"  -> return $
-                                                siConvert (v * 1000000)
-             | otherwise -> fail $ "Unknown unit '" ++ unit ++ "'"
+      in do
+        scaling <- parseUnitValue unit
+        return $ truncate (fromIntegral v * scaling)
     _ -> fail $ "Can't parse string '" ++ str ++ "'"