| Message ID | 20210211204438.1184395-5-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve do_strtosz precision | expand |
On Thu, Feb 11, 2021 at 02:44:38PM -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. > > Since qemu_strtosz() does not have an Err** parameter, and plumbing > that in would be a much larger task, we instead go with just directly > emitting the deprecation warning to stderr. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > I'm not a fan of this patch, but am proposing it for discussion purposes. Likewise. I'm *not* in favour of this patch. Allowing some fractions but not other fractions forces the potential user to figure out what the exact fraction is up front, at which point they've lost the benefit of using fractions. If users actually care about byte exact values then they already have the option to specify those exactly. If they've instead chosen to use fractions then they have implicitly decided they're ok with the potentially in-exact answer. IMHO the only question is whethe we should truncate or round, and I dont really have a preference - either is fine as long as we are intentionally picking one and documenting it. > --- > docs/system/deprecated.rst | 9 +++++++++ > tests/test-cutils.c | 6 +++--- > tests/test-keyval.c | 4 ++-- > tests/test-qemu-opts.c | 4 ++-- > util/cutils.c | 9 +++++++-- > 5 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 113c2e933f1b..2c9cb849eec5 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -154,6 +154,15 @@ Input parameters that take a size value should only use a size suffix > the value is hexadecimal. That is, '0x20M' is deprecated, and should > be written either as '32M' or as '0x2000000'. > > +inexact sizes via scaled fractions (since 6.0) > +'''''''''''''''''''''''''''''''''''''''''''''' > + > +Input parameters that take a size value should only use a fractional > +size (such as '1.5M') that will result in an exact byte value. The > +use of inexact values (such as '1.1M') that require truncation or > +rounding is deprecated, and you should instead consider writing your > +unusual size in bytes (here, '1153433' or '1153434' as desired). Regards, Daniel
On 2/23/21 11:20 AM, Daniel P. Berrangé wrote: > On Thu, Feb 11, 2021 at 02:44:38PM -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. >> >> Since qemu_strtosz() does not have an Err** parameter, and plumbing >> that in would be a much larger task, we instead go with just directly >> emitting the deprecation warning to stderr. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> >> I'm not a fan of this patch, but am proposing it for discussion purposes. > > Likewise. I'm *not* in favour of this patch. Glad we're in agreement. Consider this one dropped, and I will queue 1-3 through my NBD tree.
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 113c2e933f1b..2c9cb849eec5 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -154,6 +154,15 @@ Input parameters that take a size value should only use a size suffix the value is hexadecimal. That is, '0x20M' is deprecated, and should be written either as '32M' or as '0x2000000'. +inexact sizes via scaled fractions (since 6.0) +'''''''''''''''''''''''''''''''''''''''''''''' + +Input parameters that take a size value should only use a fractional +size (such as '1.5M') that will result in an exact byte value. The +use of inexact values (such as '1.1M') that require truncation or +rounding is deprecated, and you should instead consider writing your +unusual size in bytes (here, '1153433' or '1153434' as desired). + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/tests/test-cutils.c b/tests/test-cutils.c index bad3a6099389..c6c33866277b 100644 --- a/tests/test-cutils.c +++ b/tests/test-cutils.c @@ -2124,11 +2124,11 @@ static void test_qemu_strtosz_float(void) g_assert_cmpint(res, ==, 1024); g_assert(endptr == str + 3); - /* For convenience, we permit values that are not byte-exact */ - str = "12.345M"; + /* Fractional values should still be byte-exact */ + str = "12.125M"; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB)); + g_assert_cmpint(res, ==, (uint64_t) (12.125 * MiB)); g_assert(endptr == str + 7); } diff --git a/tests/test-keyval.c b/tests/test-keyval.c index e20c07cf3ea5..7a45c22942e6 100644 --- a/tests/test-keyval.c +++ b/tests/test-keyval.c @@ -525,7 +525,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); @@ -537,7 +537,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 6568e31a7229..549e994938fe 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -720,10 +720,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 6a8a175e0d71..1154b9de131a 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -246,12 +246,13 @@ static int64_t suffix_mul(char suffix, int64_t unit) * The size parsing supports the following syntaxes * - 12345 - decimal, scale determined by @default_suffix and @unit * - 12345{bBkKmMgGtTpPeE} - decimal, scale determined by suffix and @unit - * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, and - * fractional portion is truncated to byte + * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, if + * fractional portion is exact * - 0x7fEE - hexadecimal, unit determined by @default_suffix * * The following cause a deprecation warning, and may be removed in the future * - 0xabc{kKmMgGtTpP} - hex with scaling suffix + * - 12345.678{kKmMgGtTpPeE} - decimal with inexact fraction that caused truncation * * The following are intentionally not supported * - octal, such as 08 @@ -342,6 +343,10 @@ static int do_strtosz(const char *nptr, const char **end, retval = -ERANGE; goto out; } + if (mul_required && fraction * mul != (uint64_t) (fraction * mul)) { + warn_report("Using a fractional size that is not an exact byte " + "multiple is deprecated: %s", nptr); + } *result = val * mul + (uint64_t) (fraction * mul); retval = 0;
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. Since qemu_strtosz() does not have an Err** parameter, and plumbing that in would be a much larger task, we instead go with just directly emitting the deprecation warning to stderr. Signed-off-by: Eric Blake <eblake@redhat.com> --- I'm not a fan of this patch, but am proposing it for discussion purposes. --- docs/system/deprecated.rst | 9 +++++++++ tests/test-cutils.c | 6 +++--- tests/test-keyval.c | 4 ++-- tests/test-qemu-opts.c | 4 ++-- util/cutils.c | 9 +++++++-- 5 files changed, 23 insertions(+), 9 deletions(-)