| Message ID | 1354852190-15095-1-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 >> >>
在 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 > > > >
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
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;
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?
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().
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.
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;
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(-)