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

[Vladimir Sementsov-Ogievskiy]

04.02.2021 22:07, 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.

Honestly, I don't agree with the reasoning. User shouldn't expect something extremely 
precise when he (is it correct use "he" here, or what is the right word?) 
specify fractional size. I doubt that someone use fractional sizes for production.

Also, this will break almost everything except for 0.5.. I don't think such 
deprecation will force people to change their habits to use 0.125 instead of 
0.1. Simpler is to move to integrals and don't care.

So, in this way we can just deprecate fractional sizes at all and don't care with them 
anymore. Or allow only "0.5" fraction as the only exclusion :)

> Sadly, since qemu_strtosz() does not have an Err** parameter, we
> pollute to stderr.
>
> 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;


--
Best regards,
Vladimir