Re: [PATCH 3/3] utils: Deprecate inexact fractional suffix sizes

[Daniel P . Berrangé]

On Thu, Feb 04, 2021 at 01:07:08PM -0600, Eric Blake wrote:
> The value '1.1k' is inexact; 1126.4 bytes is not possible, so we
> happen to truncate it to 1126.  Our use of fractional sizes is
> intended for convenience, but when a user specifies a fraction that is
> not a clean translation to binary, truncating/rounding behind their
> backs can cause confusion.  Better is to deprecate inexact values,
> which still leaves '1.5k' as valid, but alerts the user to spell out
> their values as a precise byte number in cases where they are
> currently being rounded.

I don't think we should be deprecating this, as I think it makes
it very user hostile.  Users who require exact answers, won't be
using fractional syntax in the first place. IOW, by using fractional
syntax you've decided that approximation is acceptable. Given that,
I should not have to worry about whether or not the fraction I'm
using is exact or truncated. It is horrible usability to say that
"1.1k" is invalid, while "1.5k" is valid - both are valid from my
POV as a user of this.



> Note that values like '0.1G' in the testsuite need adjustment as a
> result.
> 
> Sadly, since qemu_strtosz() does not have an Err** parameter, we
> pollute to stderr.

This is only an warning, so setting an Err ** would not be appropriate
right now.

None the less we should add an Err **, because many of the callers
want an Err ** object populated, or use error_report().

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-cutils.c    | 4 ++--
>  tests/test-keyval.c    | 4 ++--
>  tests/test-qemu-opts.c | 4 ++--
>  util/cutils.c          | 4 ++++
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> index 0c2c89d6f113..ad51fb1baa51 100644
> --- a/tests/test-cutils.c
> +++ b/tests/test-cutils.c
> @@ -2095,14 +2095,14 @@ static void test_qemu_strtosz_units(void)
> 
>  static void test_qemu_strtosz_float(void)
>  {
> -    const char *str = "12.345M";
> +    const char *str = "12.125M";
>      int err;
>      const char *endptr;
>      uint64_t res = 0xbaadf00d;
> 
>      err = qemu_strtosz(str, &endptr, &res);
>      g_assert_cmpint(err, ==, 0);
> -    g_assert_cmpint(res, ==, 12.345 * MiB);
> +    g_assert_cmpint(res, ==, 12.125 * MiB);
>      g_assert(endptr == str + 7);
>  }
> 
> diff --git a/tests/test-keyval.c b/tests/test-keyval.c
> index 13be763650b2..c951ac54cd23 100644
> --- a/tests/test-keyval.c
> +++ b/tests/test-keyval.c
> @@ -522,7 +522,7 @@ static void test_keyval_visit_size(void)
>      visit_free(v);
> 
>      /* Suffixes */
> -    qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T",
> +    qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.125G,sz5=16777215T",
>                           NULL, NULL, &error_abort);
>      v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>      qobject_unref(qdict);
> @@ -534,7 +534,7 @@ static void test_keyval_visit_size(void)
>      visit_type_size(v, "sz3", &sz, &error_abort);
>      g_assert_cmphex(sz, ==, 2 * MiB);
>      visit_type_size(v, "sz4", &sz, &error_abort);
> -    g_assert_cmphex(sz, ==, GiB / 10);
> +    g_assert_cmphex(sz, ==, GiB / 8);
>      visit_type_size(v, "sz5", &sz, &error_abort);
>      g_assert_cmphex(sz, ==, 16777215ULL * TiB);
>      visit_check_struct(v, &error_abort);
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index f79b698e6715..6a1ea1d01c4f 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -717,10 +717,10 @@ static void test_opts_parse_size(void)
>      g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, 8);
>      g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 1536);
>      g_assert_cmphex(qemu_opt_get_size(opts, "size3", 0), ==, 2 * MiB);
> -    opts = qemu_opts_parse(&opts_list_02, "size1=0.1G,size2=16777215T",
> +    opts = qemu_opts_parse(&opts_list_02, "size1=0.125G,size2=16777215T",
>                             false, &error_abort);
>      g_assert_cmpuint(opts_count(opts), ==, 2);
> -    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 10);
> +    g_assert_cmphex(qemu_opt_get_size(opts, "size1", 0), ==, GiB / 8);
>      g_assert_cmphex(qemu_opt_get_size(opts, "size2", 0), ==, 16777215ULL * 
> TiB);
> 
>      /* Beyond limit with suffix */
> diff --git a/util/cutils.c b/util/cutils.c
> index 75190565cbb5..5ec6101ae778 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -327,6 +327,10 @@ static int do_strtosz(const char *nptr, const char **end,
>          retval = -ERANGE;
>          goto out;
>      }
> +    if (mul_required && fraction * mul != (uint64_t) (fraction * mul)) {
> +        fprintf(stderr, "Using a fractional size that is not an exact byte "
> +                "multiple is deprecated: %s\n", nptr);
> +    }
>      *result = val * mul + (uint64_t) (fraction * mul);
>      retval = 0;
> 
> -- 
> 2.30.0
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|