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-a-int128-t 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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs