Re: [Qemu-devel] [ [PATCH 1/2] cutils:change strtosz_suffix_unit function

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> On Thu, 13 Dec 2012 09:03:50 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Igor Mammedov <address@hidden> writes:
>> 
>> > On Fri, 7 Dec 2012 11:49:49 +0800
>> > liguang <address@hidden> wrote:
>> >
>> >> if value to be translated is larger than INT64_MAX,
>> >> this function will not be convenient for caller to
>> >> be aware of it, so change a little for this.
>> >> 
>> >> Signed-off-by: liguang <address@hidden>
>> >> ---
>> >>  cutils.c |    5 +++--
>> >>  1 files changed, 3 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/cutils.c b/cutils.c
>> >> index 4f0692f..8905b5e 100644
>> >> --- a/cutils.c
>> >> +++ b/cutils.c
>> >> @@ -219,11 +219,11 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>> >>  int64_t strtosz_suffix_unit(const char *nptr, char **end,
>> >>                              const char default_suffix, int64_t unit)
>> >>  {
>> >> -    int64_t retval = -1;
>> >> +    int64_t retval = -1, mul;
>> >>      char *endptr;
>> >>      unsigned char c;
>> >>      int mul_required = 0;
>> >> -    double val, mul, integral, fraction;
>> >> +    double val, integral, fraction;
>> >>  
>> >>      errno = 0;
>> >>      val = strtod(nptr, &endptr);
>> >> @@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char
>> >> **end, goto fail;
>> >>      }
>> >>      if ((val * mul >= INT64_MAX) || val < 0) {
>> >> +        retval = 0;
>> > Why not to add Error argument to return errors instead of using
>> > return value?
>> > That way function would be easier to generalize in future to handle whole
>> > INT64 range.

This function parses *sizes*.  Necessarily non-negative.  It returns
them as int64_t.  Fine for sizes up to 2^63-1 bytes.

>> Generalize when you have a user, not earlier.
> http://permalink.gmane.org/gmane.comp.emulators.qemu/183792

Why does that user require sizes in [2^63..2^64-1]?

>> >              And callers would check only returned error instead of return
>> > value or if 'end' == 0. 
>> 
>> Checking the return value is sufficient now.  What makes you think you
>> have to check end, too?
> I've meant *endptr == "\0" check in several callers:
> opts_type_size(), img_create(), numa_add().

That check tests something else, namely "no junk follows the number".
You cannot put that test into strtosz_suffix_unit(), because different
callers accept numbers in different contexts.