| Message ID | 20210204190708.1306296-3-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve do_strtosz precision | expand |
04.02.2021 22:07, Eric Blake wrote: > Supporting '0x20M' looks odd, particularly since we have an 'E' suffix What about also deprecating 'E' suffix? (just my problem of reviewing previous patch) > that is ambiguous between a hex digit and the extremely large exibyte > suffix, as well as a 'B' suffix for bytes. In practice, people using > hex inputs are specifying values in bytes (and would have written > 0x2000000, or possibly relied on default_suffix in the case of > qemu_strtosz_MiB), and the use of scaling suffixes makes the most > sense for inputs in decimal (where the user would write 32M). But > rather than outright dropping support for hex-with-suffix, let's > follow our deprecation policy. Sadly, since qemu_strtosz() does not > have an Err** parameter, we pollute to stderr. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > docs/system/deprecated.rst | 8 ++++++++ > util/cutils.c | 6 +++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 6ac757ed9fa7..c404c3926e6f 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -146,6 +146,14 @@ library enabled as a cryptography provider. > Neither the ``nettle`` library, or the built-in cryptography provider are > supported on FIPS enabled hosts. > > +hexadecimal sizes with scaling multipliers (since 6.0) > +'''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +Input parameters that take a size value should only use a size suffix > +(such as 'k' or 'M') when the base is written in decimal, and not when > +the value is hexadecimal. That is, '0x20M' is deprecated, and should > +be written either as '32M' or as '0x2000000'. > + > QEMU Machine Protocol (QMP) commands > ------------------------------------ > > diff --git a/util/cutils.c b/util/cutils.c > index 0234763bd70b..75190565cbb5 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -264,7 +264,7 @@ static int do_strtosz(const char *nptr, const char **end, > int retval; > const char *endptr; > unsigned char c; > - bool mul_required = false; > + bool mul_required = false, hex = false; > uint64_t val; > int64_t mul; > double fraction = 0.0; > @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char **end, you forget to set hex to true in corresponding if(){...} > c = *endptr; > mul = suffix_mul(c, unit); > if (mul > 0) { > + if (hex) { > + fprintf(stderr, "Using a multiplier suffix on hex numbers " > + "is deprecated: %s\n", nptr); > + } > endptr++; > } else { > mul = suffix_mul(default_suffix, unit); should we also deprecate hex where default_suffix is not 'B' ?
On Fri, Feb 05, 2021 at 01:25:18PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 04.02.2021 22:07, Eric Blake wrote: > >Supporting '0x20M' looks odd, particularly since we have an 'E' suffix > > What about also deprecating 'E' suffix? (just my problem of reviewing previous patch) Ha! What if people want to specify exabytes?! That actually works at the moment: $ nbdkit memory 7E --run 'qemu-io -f raw -c "r -v 1E 512" "$uri"' Rich.
On Thu, Feb 04, 2021 at 01:07:07PM -0600, Eric Blake wrote: > Supporting '0x20M' looks odd, particularly since we have an 'E' suffix > that is ambiguous between a hex digit and the extremely large exibyte > suffix, as well as a 'B' suffix for bytes. In practice, people using > hex inputs are specifying values in bytes (and would have written > 0x2000000, or possibly relied on default_suffix in the case of > qemu_strtosz_MiB), and the use of scaling suffixes makes the most > sense for inputs in decimal (where the user would write 32M). But > rather than outright dropping support for hex-with-suffix, let's > follow our deprecation policy. Sadly, since qemu_strtosz() does not > have an Err** parameter, we pollute to stderr. Err** is only appropriate for errors, not warnings, so this statement can be removed. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > docs/system/deprecated.rst | 8 ++++++++ > util/cutils.c | 6 +++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 6ac757ed9fa7..c404c3926e6f 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -146,6 +146,14 @@ library enabled as a cryptography provider. > Neither the ``nettle`` library, or the built-in cryptography provider are > supported on FIPS enabled hosts. > > +hexadecimal sizes with scaling multipliers (since 6.0) > +'''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +Input parameters that take a size value should only use a size suffix > +(such as 'k' or 'M') when the base is written in decimal, and not when > +the value is hexadecimal. That is, '0x20M' is deprecated, and should > +be written either as '32M' or as '0x2000000'. > + > QEMU Machine Protocol (QMP) commands > ------------------------------------ > > diff --git a/util/cutils.c b/util/cutils.c > index 0234763bd70b..75190565cbb5 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -264,7 +264,7 @@ static int do_strtosz(const char *nptr, const char **end, > int retval; > const char *endptr; > unsigned char c; > - bool mul_required = false; > + bool mul_required = false, hex = false; > uint64_t val; > int64_t mul; > double fraction = 0.0; > @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char **end, > c = *endptr; > mul = suffix_mul(c, unit); > if (mul > 0) { > + if (hex) { > + fprintf(stderr, "Using a multiplier suffix on hex numbers " > + "is deprecated: %s\n", nptr); warn_report(), since IIUC, that'll get into HMP response correctly. > + } > endptr++; > } else { > mul = suffix_mul(default_suffix, unit); Regards, Daniel
On 2/5/21 4:25 AM, Vladimir Sementsov-Ogievskiy wrote: > 04.02.2021 22:07, Eric Blake wrote: >> Supporting '0x20M' looks odd, particularly since we have an 'E' suffix > > What about also deprecating 'E' suffix? (just my problem of reviewing > previous patch) No, we want to keep '1E' as a valid way to spell 1 exabyte. That has uses in the wild (admittedly corner-case, as that is a LOT of storage). It is only '0x1E' where the use of 'E' as a hex digit has priority over 'E' as a suffix meaning exabyte. > >> that is ambiguous between a hex digit and the extremely large exibyte >> suffix, as well as a 'B' suffix for bytes. In practice, people using >> hex inputs are specifying values in bytes (and would have written >> 0x2000000, or possibly relied on default_suffix in the case of >> qemu_strtosz_MiB), and the use of scaling suffixes makes the most >> sense for inputs in decimal (where the user would write 32M). But >> rather than outright dropping support for hex-with-suffix, let's >> follow our deprecation policy. Sadly, since qemu_strtosz() does not >> have an Err** parameter, we pollute to stderr. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> +++ b/util/cutils.c >> @@ -264,7 +264,7 @@ static int do_strtosz(const char *nptr, const char >> **end, >> int retval; >> const char *endptr; >> unsigned char c; >> - bool mul_required = false; >> + bool mul_required = false, hex = false; >> uint64_t val; >> int64_t mul; >> double fraction = 0.0; >> @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const >> char **end, > > you forget to set hex to true in corresponding if(){...} > >> c = *endptr; >> mul = suffix_mul(c, unit); >> if (mul > 0) { >> + if (hex) { >> + fprintf(stderr, "Using a multiplier suffix on hex numbers " >> + "is deprecated: %s\n", nptr); >> + } D'oh. Now I get to rerun my tests to see when the warning triggers. >> endptr++; >> } else { >> mul = suffix_mul(default_suffix, unit); > > should we also deprecate hex where default_suffix is not 'B' ? That's exactly what this patch is (supposed to be) doing. If we parsed a hex number, and there was an explicit suffix at all (which is necessarily neither 'B' nor 'E', since those were already consumed while parsing the hex number), issue a warning.
On 2/5/21 5:13 AM, Daniel P. Berrangé wrote: > On Thu, Feb 04, 2021 at 01:07:07PM -0600, Eric Blake wrote: >> Supporting '0x20M' looks odd, particularly since we have an 'E' suffix >> that is ambiguous between a hex digit and the extremely large exibyte >> suffix, as well as a 'B' suffix for bytes. In practice, people using >> hex inputs are specifying values in bytes (and would have written >> 0x2000000, or possibly relied on default_suffix in the case of >> qemu_strtosz_MiB), and the use of scaling suffixes makes the most >> sense for inputs in decimal (where the user would write 32M). But >> rather than outright dropping support for hex-with-suffix, let's >> follow our deprecation policy. Sadly, since qemu_strtosz() does not >> have an Err** parameter, we pollute to stderr. > > Err** is only appropriate for errors, not warnings, so this statement > can be removed. The point of the comment was that we have no other mechanism for reporting the deprecation other than polluting to stderr. And if we ever remove the support, we'll either have to silently reject input that we used to accept, or plumb through Err** handling to allow better reporting of WHY we are rejecting input. But I didn't feel like adding Err** support now. >> @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char **end, >> c = *endptr; >> mul = suffix_mul(c, unit); >> if (mul > 0) { >> + if (hex) { >> + fprintf(stderr, "Using a multiplier suffix on hex numbers " >> + "is deprecated: %s\n", nptr); > > warn_report(), since IIUC, that'll get into HMP response correctly. Yes, that sounds better. I'll use that in v2, as well as tweaking the commit message. > >> + } >> endptr++; >> } else { >> mul = suffix_mul(default_suffix, unit); > > Regards, > Daniel >
On Fri, Feb 05, 2021 at 07:40:36AM -0600, Eric Blake wrote: > On 2/5/21 5:13 AM, Daniel P. Berrangé wrote: > > On Thu, Feb 04, 2021 at 01:07:07PM -0600, Eric Blake wrote: > >> Supporting '0x20M' looks odd, particularly since we have an 'E' suffix > >> that is ambiguous between a hex digit and the extremely large exibyte > >> suffix, as well as a 'B' suffix for bytes. In practice, people using > >> hex inputs are specifying values in bytes (and would have written > >> 0x2000000, or possibly relied on default_suffix in the case of > >> qemu_strtosz_MiB), and the use of scaling suffixes makes the most > >> sense for inputs in decimal (where the user would write 32M). But > >> rather than outright dropping support for hex-with-suffix, let's > >> follow our deprecation policy. Sadly, since qemu_strtosz() does not > >> have an Err** parameter, we pollute to stderr. > > > > Err** is only appropriate for errors, not warnings, so this statement > > can be removed. > > The point of the comment was that we have no other mechanism for > reporting the deprecation other than polluting to stderr. And if we > ever remove the support, we'll either have to silently reject input that > we used to accept, or plumb through Err** handling to allow better > reporting of WHY we are rejecting input. But I didn't feel like adding > Err** support now. Yeah, I guess what I meant was that warning on stderr is the expected standard practice for deprecations. We only need to worry about other thing once the deprecation turns into a hard error later. > > >> @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char **end, > >> c = *endptr; > >> mul = suffix_mul(c, unit); > >> if (mul > 0) { > >> + if (hex) { > >> + fprintf(stderr, "Using a multiplier suffix on hex numbers " > >> + "is deprecated: %s\n", nptr); > > > > warn_report(), since IIUC, that'll get into HMP response correctly. > > Yes, that sounds better. I'll use that in v2, as well as tweaking the > commit message. > > > > >> + } > >> endptr++; > >> } else { > >> mul = suffix_mul(default_suffix, unit); > > > > Regards, > > Daniel > > > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org Regards, Daniel
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 6ac757ed9fa7..c404c3926e6f 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -146,6 +146,14 @@ library enabled as a cryptography provider. Neither the ``nettle`` library, or the built-in cryptography provider are supported on FIPS enabled hosts. +hexadecimal sizes with scaling multipliers (since 6.0) +'''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Input parameters that take a size value should only use a size suffix +(such as 'k' or 'M') when the base is written in decimal, and not when +the value is hexadecimal. That is, '0x20M' is deprecated, and should +be written either as '32M' or as '0x2000000'. + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/util/cutils.c b/util/cutils.c index 0234763bd70b..75190565cbb5 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -264,7 +264,7 @@ static int do_strtosz(const char *nptr, const char **end, int retval; const char *endptr; unsigned char c; - bool mul_required = false; + bool mul_required = false, hex = false; uint64_t val; int64_t mul; double fraction = 0.0; @@ -309,6 +309,10 @@ static int do_strtosz(const char *nptr, const char **end, c = *endptr; mul = suffix_mul(c, unit); if (mul > 0) { + if (hex) { + fprintf(stderr, "Using a multiplier suffix on hex numbers " + "is deprecated: %s\n", nptr); + } endptr++; } else { mul = suffix_mul(default_suffix, unit);
Supporting '0x20M' looks odd, particularly since we have an 'E' suffix that is ambiguous between a hex digit and the extremely large exibyte suffix, as well as a 'B' suffix for bytes. In practice, people using hex inputs are specifying values in bytes (and would have written 0x2000000, or possibly relied on default_suffix in the case of qemu_strtosz_MiB), and the use of scaling suffixes makes the most sense for inputs in decimal (where the user would write 32M). But rather than outright dropping support for hex-with-suffix, let's follow our deprecation policy. Sadly, since qemu_strtosz() does not have an Err** parameter, we pollute to stderr. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/system/deprecated.rst | 8 ++++++++ util/cutils.c | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-)