Patchwork [[PATCH,1/2] cutils:change strtosz_suffix_unit function

login
register
mail settings
Submitter liguang
Date Dec. 7, 2012, 3:49 a.m.
Message ID <1354852190-15095-1-git-send-email-lig.fnst@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/204383/
State New
Headers show

Comments

liguang - Dec. 7, 2012, 3:49 a.m.
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(-)
Stefan Hajnoczi - Dec. 11, 2012, 9:52 a.m.
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
> 
>
Markus Armbruster - Dec. 11, 2012, 3:06 p.m.
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
>> 
>>
liguang - Dec. 12, 2012, 12:58 a.m.
在 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
> > 
> >
Stefan Hajnoczi - Dec. 12, 2012, 8:18 a.m.
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
Igor Mammedov - Dec. 12, 2012, 10:12 a.m.
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;
Markus Armbruster - Dec. 13, 2012, 8:03 a.m.
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?
Igor Mammedov - Dec. 13, 2012, 9:11 a.m.
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().
Markus Armbruster - Dec. 13, 2012, 12:09 p.m.
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.

Patch

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;