Re: [Libguestfs] [PATCH nbdkit] server: Move size parsing code (nbdkit_parse_size) to common/include
On Tue, Sep 05, 2023 at 11:09:02AM +0100, Richard W.M. Jones wrote:
> > +static inline int64_t
> > +human_size_parse (const char *str,
> > + const char **error, const char **pstr)
> > +{
> > + int64_t size;
> > + char *end;
> > + uint64_t scale = 1;
> > +
> > + /* XXX Should we also parse things like '1.5M'? */
> > + /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
> > + * because some of them are valid hex digits.
> > + */
> > + errno = 0;
> > + size = strtoimax (str, &end, 10);
>
> (1) A further improvement here (likely best done in a separate patch)
> could be to change the type of "size" to "intmax_t", from
"int64_t".
> That way, the assignment will be safe even theoretically, *and* the
> overflow check at the bottom of the function (with the division &
> comparison of the quotient against INT_MAX) will work just the same.
I'm always very unsure how this all works. In particular I seem to
recall that intmax_t is no longer really the maximum possible int
(because of int128) and so it's always 64 bit on platforms we care
about. Can Eric comment?
intmax_t was supposed to be whatever the compiler supports as its
largest integer type; but you are right that when gcc added
__int128_t, they did NOT change intmax_t at the time (arguably by
using weasel-words that as an implementation-defined type, it was not
an integer type merely because you can't write integer literals of
that type, even though it behaves integral in every other aspect).
We're kind of in a frozen state where making intmax_t larger than 64
bits will break more programs than expected because it has ABI
implications:
https://stackoverflow.com/questions/21265462/why-in-g-stdintmax-t-is-not-...
My personal preference is to avoid intmax_t, as it has too much
baggage (the risk of future widening, vs. NOT being the widest type
after all), similar to how size_t already has baggage. In short,
having something that is not platform specific is easier to reason
about (for the same way that using size_t gives me more grief than
directly using int32_t or int64_t; even though size_t is such a
naturally occuring type, the fact that it is not uniform width makes
it trickier to work with).
> > +
> > + if (INT64_MAX / scale < size) {
> > + *error = "could not parse size: size * scale overflows";
> > + *pstr = str;
> > + return -1;
> > + }
And thus I prefer that this comparison stay pegged to INT64_MAX, and
not INT_MAX.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org