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

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

[liguang]

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 <lig.fnst@cn.fujitsu.com>
---
 cutils.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/cutils.c b/cutils.c
index 4f0692f..da05c9e 100644
--- a/cutils.c
+++ b/cutils.c
@@ -219,7 +219,7 @@ 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 = EINVAL;
     char *endptr;
     unsigned char c;
     int mul_required = 0;
@@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
         goto fail;
     }
     if ((val * mul >= INT64_MAX) || val < 0) {
+        retval = ERANGE;
         goto fail;
     }
     retval = val * mul;
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/2] qemu-img:report size overflow error message

[liguang]

qemu-img will complain when qcow or qcow2
size overflow for 64 bits, report the right
message in this condition.

before change:
qemu-img: Invalid image size specified! You may use k, M, G or T suffixes for
qemu-img: kilobytes, megabytes, gigabytes and terabytes.

after change:
qemu-img: Image size must be less than 8 exabytes!

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 qemu-img.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e29e01b..1c3af67 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -346,13 +346,18 @@ static int img_create(int argc, char **argv)
         int64_t sval;
         char *end;
         sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
-        if (sval < 0 || *end) {
+        if (sval == EINVAL || *end) {
             error_report("Invalid image size specified! You may use k, M, G or "
                   "T suffixes for ");
             error_report("kilobytes, megabytes, gigabytes and terabytes.");
             ret = -1;
             goto out;
         }
+        if (sval == ERANGE) {
+            error_report("Image size must be less than 8 exabytes!");
+            ret = -1;
+            goto out;
+        }
         img_size = (uint64_t)sval;
     }
 
-- 
1.7.2.5

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

[Markus Armbruster]

liguang <lig.fnst@cn.fujitsu.com> writes:

> 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 <lig.fnst@cn.fujitsu.com>
> ---
>  cutils.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 4f0692f..da05c9e 100644
> --- a/cutils.c
> +++ b/cutils.c
   /*
    * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
    * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
    * in *end, if not NULL. Return -1 on error.
    */
> @@ -219,7 +219,7 @@ 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 = EINVAL;
>      char *endptr;
>      unsigned char c;
>      int mul_required = 0;
> @@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
>          goto fail;
>      }
>      if ((val * mul >= INT64_MAX) || val < 0) {
> +        retval = ERANGE;
>          goto fail;
>      }
>      retval = val * mul;

Your error codes aren't negative, and you failed to update the function
comment!

Re: [Qemu-devel] [PATCH 2/2] qemu-img:report size overflow error message

[Markus Armbruster]

liguang <lig.fnst@cn.fujitsu.com> writes:

> qemu-img will complain when qcow or qcow2
> size overflow for 64 bits, report the right
> message in this condition.
>
> before change:
> qemu-img: Invalid image size specified! You may use k, M, G or T suffixes for
> qemu-img: kilobytes, megabytes, gigabytes and terabytes.
>
> after change:
> qemu-img: Image size must be less than 8 exabytes!
>
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  qemu-img.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index e29e01b..1c3af67 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -346,13 +346,18 @@ static int img_create(int argc, char **argv)
>          int64_t sval;
>          char *end;
>          sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
> -        if (sval < 0 || *end) {
> +        if (sval == EINVAL || *end) {
>              error_report("Invalid image size specified! You may use k, M, G or "
>                    "T suffixes for ");
>              error_report("kilobytes, megabytes, gigabytes and terabytes.");
>              ret = -1;
>              goto out;
>          }
> +        if (sval == ERANGE) {
> +            error_report("Image size must be less than 8 exabytes!");
> +            ret = -1;
> +            goto out;
> +        }
>          img_size = (uint64_t)sval;
>      }

If strtosz_suffix() ever acquires additional error codes, this caller
will fail to detect the failure.  Try something like

        if (sval < 0 || *end) {
            if (sval == -ERANGE) {
                error_report("Image size must be less than 8 exabytes!");
            } else {
                error_report("Invalid image size specified!"
                      " You may use k, M, G or T suffixes for");
                error_report("kilobytes, megabytes, gigabytes and terabytes.");
            }
            ret = -1;
            goto out;
        }

To be pedantically correct, "8 exabytes" should be "8 Exbibytes" or "8
EiB".

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

[Igor Mammedov]

On Fri, 14 Dec 2012 13:09:17 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> liguang <lig.fnst@cn.fujitsu.com> writes:
> 
> > 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 <lig.fnst@cn.fujitsu.com>
> > ---
> >  cutils.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/cutils.c b/cutils.c
> > index 4f0692f..da05c9e 100644
> > --- a/cutils.c
> > +++ b/cutils.c
>    /*
>     * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
>     * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
>     * in *end, if not NULL. Return -1 on error.
>     */
Size is not the only user of it, this function is used to convert KHz,... in
target-i386/cpu.c, i.e. unit might be something else than 1024. That's why
there is question about generalizing strtosz_suffix_unit() in future instead of
duplicating suffixed int parsing. And specialization for size with
unit=1024 and enforcing limits could be made in strtosz_suffix() which is
used by current size users.


> > @@ -219,7 +219,7 @@ 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 = EINVAL;
> >      char *endptr;
> >      unsigned char c;
> >      int mul_required = 0;
> > @@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
> >          goto fail;
> >      }
> >      if ((val * mul >= INT64_MAX) || val < 0) {
> > +        retval = ERANGE;
> >          goto fail;
> >      }
> >      retval = val * mul;
> 
> Your error codes aren't negative, and you failed to update the function
> comment!
Returning negative here is fine for now form target-i386/cpu.c pov.


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 2/2] qemu-img:report size overflow error message

[li guang]

在 2012-12-14五的 13:19 +0100，Markus Armbruster写道：
> liguang <lig.fnst@cn.fujitsu.com> writes:
> 
> > qemu-img will complain when qcow or qcow2
> > size overflow for 64 bits, report the right
> > message in this condition.
> >
> > before change:
> > qemu-img: Invalid image size specified! You may use k, M, G or T suffixes for
> > qemu-img: kilobytes, megabytes, gigabytes and terabytes.
> >
> > after change:
> > qemu-img: Image size must be less than 8 exabytes!
> >
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  qemu-img.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> >
> > diff --git a/qemu-img.c b/qemu-img.c
> > index e29e01b..1c3af67 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -346,13 +346,18 @@ static int img_create(int argc, char **argv)
> >          int64_t sval;
> >          char *end;
> >          sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
> > -        if (sval < 0 || *end) {
> > +        if (sval == EINVAL || *end) {
> >              error_report("Invalid image size specified! You may use k, M, G or "
> >                    "T suffixes for ");
> >              error_report("kilobytes, megabytes, gigabytes and terabytes.");
> >              ret = -1;
> >              goto out;
> >          }
> > +        if (sval == ERANGE) {
> > +            error_report("Image size must be less than 8 exabytes!");
> > +            ret = -1;
> > +            goto out;
> > +        }
> >          img_size = (uint64_t)sval;
> >      }
> 
> If strtosz_suffix() ever acquires additional error codes, this caller
> will fail to detect the failure.  Try something like
> 
>         if (sval < 0 || *end) {
>             if (sval == -ERANGE) {
>                 error_report("Image size must be less than 8 exabytes!");
>             } else {
>                 error_report("Invalid image size specified!"
>                       " You may use k, M, G or T suffixes for");
>                 error_report("kilobytes, megabytes, gigabytes and terabytes.");
>             }
>             ret = -1;
>             goto out;
>         }
> 
> To be pedantically correct, "8 exabytes" should be "8 Exbibytes" or "8
> EiB".

yes, it's better.
Thanks!

-- 
regards!
li guang

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

[li guang]

在 2012-12-14五的 13:09 +0100，Markus Armbruster写道：
> liguang <lig.fnst@cn.fujitsu.com> writes:
> 
> > 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 <lig.fnst@cn.fujitsu.com>
> > ---
> >  cutils.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/cutils.c b/cutils.c
> > index 4f0692f..da05c9e 100644
> > --- a/cutils.c
> > +++ b/cutils.c
>    /*
>     * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
>     * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
>     * in *end, if not NULL. Return -1 on error.
>     */
> > @@ -219,7 +219,7 @@ 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 = EINVAL;
> >      char *endptr;
> >      unsigned char c;
> >      int mul_required = 0;
> > @@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
> >          goto fail;
> >      }
> >      if ((val * mul >= INT64_MAX) || val < 0) {
> > +        retval = ERANGE;
> >          goto fail;
> >      }
> >      retval = val * mul;
> 
> Your error codes aren't negative, and you failed to update the function
> comment!
OK, will fix.
-- 
regards!
li guang

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

[li guang]

在 2012-12-14五的 14:45 +0100，Igor Mammedov写道：
> On Fri, 14 Dec 2012 13:09:17 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > liguang <lig.fnst@cn.fujitsu.com> writes:
> > 
> > > 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 <lig.fnst@cn.fujitsu.com>
> > > ---
> > >  cutils.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/cutils.c b/cutils.c
> > > index 4f0692f..da05c9e 100644
> > > --- a/cutils.c
> > > +++ b/cutils.c
> >    /*
> >     * Convert string to bytes, allowing either B/b for bytes, K/k for KB,
> >     * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned
> >     * in *end, if not NULL. Return -1 on error.
> >     */
> Size is not the only user of it, this function is used to convert KHz,... in
> target-i386/cpu.c, i.e. unit might be something else than 1024. That's why
> there is question about generalizing strtosz_suffix_unit() in future instead of
> duplicating suffixed int parsing. And specialization for size with
> unit=1024 and enforcing limits could be made in strtosz_suffix() which is
> used by current size users.
> 
> 
> > > @@ -219,7 +219,7 @@ 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 = EINVAL;
> > >      char *endptr;
> > >      unsigned char c;
> > >      int mul_required = 0;
> > > @@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end,
> > >          goto fail;
> > >      }
> > >      if ((val * mul >= INT64_MAX) || val < 0) {
> > > +        retval = ERANGE;
> > >          goto fail;
> > >      }
> > >      retval = val * mul;
> > 
> > Your error codes aren't negative, and you failed to update the function
> > comment!
> Returning negative here is fine for now form target-i386/cpu.c pov.

OK


-- 
regards!
li guang

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

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 13 Dec 2012 09:03:50 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@redhat.com> writes:
>> 
>> > On Fri, 7 Dec 2012 11:49:49 +0800
>> > liguang <lig.fnst@cn.fujitsu.com> 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 <lig.fnst@cn.fujitsu.com>
>> >> ---
>> >>  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.

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

[Igor Mammedov]

On Thu, 13 Dec 2012 09:03:50 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Fri, 7 Dec 2012 11:49:49 +0800
> > liguang <lig.fnst@cn.fujitsu.com> 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 <lig.fnst@cn.fujitsu.com>
> >> ---
> >>  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.
> 
> Generalize when you have a user, not earlier.
http://permalink.gmane.org/gmane.comp.emulators.qemu/183792

> 
> >              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().


-- 
Regards,
  Igor

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

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Fri, 7 Dec 2012 11:49:49 +0800
> liguang <lig.fnst@cn.fujitsu.com> 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 <lig.fnst@cn.fujitsu.com>
>> ---
>>  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.

Generalize when you have a user, not earlier.

>              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?

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

[Igor Mammedov]

On Fri, 7 Dec 2012 11:49:49 +0800
liguang <lig.fnst@cn.fujitsu.com> 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 <lig.fnst@cn.fujitsu.com>
> ---
>  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. And callers would check only returned error instead of return
value or if 'end' == 0. 

>          goto fail;
>      }
>      retval = val * mul;

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

[Stefan Hajnoczi]

On Wed, Dec 12, 2012 at 08:58:17AM +0800, li guang wrote:
> 在 2012-12-11二的 10:52 +0100，Stefan Hajnoczi写道：
> > On Fri, Dec 07, 2012 at 11:49:49AM +0800, liguang 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 <lig.fnst@cn.fujitsu.com>
> > > ---
> > >  cutils.c |    5 +++--
> > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > I don't understand what this patch is supposed to do.
> 
> little change to let the caller know the translated value overflow

Using a negative errno will make the intent clear.

> > 
> > Why change the type of mul from double to int64_t?
> 
> mul is multiplier generated by suffix_mul(return value type int64_t)
> so double is incorrect.

Okay, thanks.  I asked because I wondered whether it had something to do
with overflow detection.  Please put unrelated changes in separate
patches - and if you feel it's not worth putting in a separate patch
then maybe it's not worth changing at all.

Stefan

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

[li guang]

在 2012-12-11二的 10:52 +0100，Stefan Hajnoczi写道：
> On Fri, Dec 07, 2012 at 11:49:49AM +0800, liguang 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 <lig.fnst@cn.fujitsu.com>
> > ---
> >  cutils.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> I don't understand what this patch is supposed to do.

little change to let the caller know the translated value overflow

> 
> Why change the type of mul from double to int64_t?

mul is multiplier generated by suffix_mul(return value type int64_t)
so double is incorrect.

> 
> Are you using retval = 0 as a special value to indicate overflow?  

yes, just an indicator for caller, not so normal return value

> Then
> the new return value should be documented.  But it would be better to
> change the function to return -errno values instead of 0/-1 so the
> caller knows the reason for specific failure cases (e.g. -E2BIG).

because of 'retval = 0' is only a hacking way to reflect an illegal
translated value, does it have to act in such an 'official' way?

> 
> Should the val < 0 case be checked earlier in the function?  It seems
> like a different failure case then INT64_MAX overflow.

maybe, but I think no one interested in difference between
under-overflow

> 
> > 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;
> >          goto fail;
> >      }
> >      retval = val * mul;
> > -- 
> > 1.7.2.5
> > 
> > 

-- 
regards!
li guang

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, Dec 07, 2012 at 11:49:49AM +0800, liguang 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 <lig.fnst@cn.fujitsu.com>
>> ---
>>  cutils.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> I don't understand what this patch is supposed to do.
>
> Why change the type of mul from double to int64_t?
>
> Are you using retval = 0 as a special value to indicate overflow?  Then
> the new return value should be documented.

Zero is a legitimate value, so it cannot be used to indicate overflow.

>                                             But it would be better to
> change the function to return -errno values instead of 0/-1 so the
> caller knows the reason for specific failure cases (e.g. -E2BIG).

The appropriate error code is ERANGE, following strtol() precedence.

Nitpick: function doesn't return zero on succes, it returns a
non-negative value.

> Should the val < 0 case be checked earlier in the function?  It seems
> like a different failure case then INT64_MAX overflow.

Only if callers are interested in differentiating between under- and
overflow.  Which I doubt.

>> 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;
>>          goto fail;
>>      }
>>      retval = val * mul;
>> -- 
>> 1.7.2.5
>> 
>> 

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

[Stefan Hajnoczi]

On Fri, Dec 07, 2012 at 11:49:49AM +0800, liguang 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 <lig.fnst@cn.fujitsu.com>
> ---
>  cutils.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)

I don't understand what this patch is supposed to do.

Why change the type of mul from double to int64_t?

Are you using retval = 0 as a special value to indicate overflow?  Then
the new return value should be documented.  But it would be better to
change the function to return -errno values instead of 0/-1 so the
caller knows the reason for specific failure cases (e.g. -E2BIG).

Should the val < 0 case be checked earlier in the function?  It seems
like a different failure case then INT64_MAX overflow.

> 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;
>          goto fail;
>      }
>      retval = val * mul;
> -- 
> 1.7.2.5
> 
> 

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

[liguang]

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 <lig.fnst@cn.fujitsu.com>
---
 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;
         goto fail;
     }
     retval = val * mul;
-- 
1.7.2.5