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

[Richard W.M. Jones]

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.
> 
> 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.

Does "deprecate" mean there's a plan to eventually remove this?  If so
I think it should say that (in docs/system/deprecated.rst I think).

I think it would be better to remove this now or in the future since
it's a trap for users.

Rich.

> 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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW