[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 2/2] qemu-img:report size overflow error message

[li guang]

在 2012-12-11二的 16:11 +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.
> 
> Recommend to show the reproducer with output before and after the patch
> in the commit message.

OK, reasonable.
Thanks!

-- 
regards!
li guang

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.

Recommend to show the reproducer with output before and after the patch
in the commit message.

[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.

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

diff --git a/qemu-img.c b/qemu-img.c
index e29e01b..f3209b4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -353,6 +353,11 @@ static int img_create(int argc, char **argv)
             ret = -1;
             goto out;
         }
+        if (sval == 0) {
+            error_report("Image size must be less than 8 exabytes!");
+            ret = -1;
+            goto out;
+        }
         img_size = (uint64_t)sval;
     }
 
-- 
1.7.2.5