Patchwork [1/5] Introduce strtobytes() library function to convert string to byte count.

login
register
mail settings
Submitter Jes Sorensen
Date Sept. 15, 2010, 12:23 p.m.
Message ID <1284553440-17985-2-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/64803/
State New
Headers show

Comments

Jes Sorensen - Sept. 15, 2010, 12:23 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c      |   39 +++++++++++++++++++++++++++++++++++++++
 qemu-common.h |    1 +
 vl.c          |   26 +++++++-------------------
 3 files changed, 47 insertions(+), 19 deletions(-)
Andreas Färber - Sept. 15, 2010, 6:46 p.m.
Am 15.09.2010 um 14:23 schrieb Jes.Sorensen@redhat.com:
[...]
> +/*
> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
> + * G/b for GB or T/t for TB. Default without any postfix is MB.
       ^^^ typo
> + * End pointer will be returned in *end, if end is valid.
> + * Return 0 on error.
> + */

You seem to be refactoring existing code into this function, but the  
use of such suffixes usually brings up the question whether it's  
factor 1024 or 1000.
Here you're using 1024 apparently. If you don't want the user dealing  
with (imo ugly) Ki/Mi/Gi/Ti units this should at least be documented  
accordingly: G/g for GiB, etc. or G/g for GB = 1024 MB, etc.

Andreas

> +uint64_t strtobytes(const char *nptr, char **end)
> +{
> +    uint64_t value;
> +    char *endptr;
> +
> +    value = strtoll(nptr, &endptr, 0);
> +    switch (*endptr++) {
> +    case 'K':
> +    case 'k':
> +        value <<= 10;
> +        break;
> +    case 0:
> +    case 'M':
> +    case 'm':
> +        value <<= 20;
> +        break;
> +    case 'G':
> +    case 'g':
> +        value <<= 30;
> +        break;
> +    case 'T':
> +    case 't':
> +        value <<= 40;
> +        break;
> +    default:
> +        value = 0;
> +    }
> +
> +    if (end)
> +        *end = endptr;
> +
> +    return value;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index dfd3dc0..01d7dcb 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -137,6 +137,7 @@ time_t mktimegm(struct tm *tm);
> int qemu_fls(int i);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> +uint64_t strtobytes(const char *nptr, char **end);
>
> /* path.c */
> void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index 3f45aa9..0cdd94a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,14 +734,10 @@ static void numa_add(const char *optarg)
>         if (get_param_value(option, 128, "mem", optarg) == 0) {
>             node_mem[nodenr] = 0;
>         } else {
> -            value = strtoull(option, &endptr, 0);
> -            switch (*endptr) {
> -            case 0: case 'M': case 'm':
> -                value <<= 20;
> -                break;
> -            case 'G': case 'g':
> -                value <<= 30;
> -                break;
> +            value = strtobytes(option, NULL);
> +            if (!value) {
> +                fprintf(stderr, "qemu: invalid numa mem size: %s 
> \n", optarg);
> +                exit(1);
>             }
>             node_mem[nodenr] = value;
>         }
> @@ -2166,17 +2162,9 @@ int main(int argc, char **argv, char **envp)
>                 break;
>             case QEMU_OPTION_m: {
>                 uint64_t value;
> -                char *ptr;
>
> -                value = strtoul(optarg, &ptr, 10);
> -                switch (*ptr) {
> -                case 0: case 'M': case 'm':
> -                    value <<= 20;
> -                    break;
> -                case 'G': case 'g':
> -                    value <<= 30;
> -                    break;
> -                default:
> +                value = strtobytes(optarg, NULL);
> +                if (!value) {
>                     fprintf(stderr, "qemu: invalid ram size: %s\n",  
> optarg);
>                     exit(1);
>                 }
> -- 
> 1.7.2.2
Stefan Weil - Sept. 15, 2010, 8:50 p.m.
Am 15.09.2010 20:46, schrieb Andreas Färber:
> Am 15.09.2010 um 14:23 schrieb Jes.Sorensen@redhat.com:
> [...]
>> +/*
>> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
>> + * G/b for GB or T/t for TB. Default without any postfix is MB.
>       ^^^ typo
>> + * End pointer will be returned in *end, if end is valid.
>> + * Return 0 on error.
>> + */
>
> You seem to be refactoring existing code into this function, but the 
> use of such suffixes usually brings up the question whether it's 
> factor 1024 or 1000.
> Here you're using 1024 apparently. If you don't want the user dealing 
> with (imo ugly) Ki/Mi/Gi/Ti units this should at least be documented 
> accordingly: G/g for GiB, etc. or G/g for GB = 1024 MB, etc.
>
> Andreas
[snip]

I'd prefer the standard prefixes: KiB, MiB, GiB for powers of 1024, KB, 
MB, GB for powers of 1000.
The standard has the big advantage of being a standard, even if not 
everybody likes it.

Existing QEMU code should be cleaned (= changed) were needed.

Stefan

Patch

diff --git a/cutils.c b/cutils.c
index 036ae3c..a3087fe 100644
--- a/cutils.c
+++ b/cutils.c
@@ -251,3 +251,42 @@  int fcntl_setfl(int fd, int flag)
 }
 #endif
 
+/*
+ * Convert string to bytes, allowing either K/k for KB, M/m for MB,
+ * G/b for GB or T/t for TB. Default without any postfix is MB.
+ * End pointer will be returned in *end, if end is valid.
+ * Return 0 on error.
+ */
+uint64_t strtobytes(const char *nptr, char **end)
+{
+    uint64_t value;
+    char *endptr;
+
+    value = strtoll(nptr, &endptr, 0);
+    switch (*endptr++) {
+    case 'K':
+    case 'k':
+        value <<= 10;
+        break;
+    case 0:
+    case 'M':
+    case 'm':
+        value <<= 20;
+        break;
+    case 'G':
+    case 'g':
+        value <<= 30;
+        break;
+    case 'T':
+    case 't':
+        value <<= 40;
+        break;
+    default:
+        value = 0;
+    }
+
+    if (end)
+        *end = endptr;
+
+    return value;
+}
diff --git a/qemu-common.h b/qemu-common.h
index dfd3dc0..01d7dcb 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -137,6 +137,7 @@  time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
+uint64_t strtobytes(const char *nptr, char **end);
 
 /* path.c */
 void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index 3f45aa9..0cdd94a 100644
--- a/vl.c
+++ b/vl.c
@@ -734,14 +734,10 @@  static void numa_add(const char *optarg)
         if (get_param_value(option, 128, "mem", optarg) == 0) {
             node_mem[nodenr] = 0;
         } else {
-            value = strtoull(option, &endptr, 0);
-            switch (*endptr) {
-            case 0: case 'M': case 'm':
-                value <<= 20;
-                break;
-            case 'G': case 'g':
-                value <<= 30;
-                break;
+            value = strtobytes(option, NULL);
+            if (!value) {
+                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+                exit(1);
             }
             node_mem[nodenr] = value;
         }
@@ -2166,17 +2162,9 @@  int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_m: {
                 uint64_t value;
-                char *ptr;
 
-                value = strtoul(optarg, &ptr, 10);
-                switch (*ptr) {
-                case 0: case 'M': case 'm':
-                    value <<= 20;
-                    break;
-                case 'G': case 'g':
-                    value <<= 30;
-                    break;
-                default:
+                value = strtobytes(optarg, NULL);
+                if (!value) {
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);
                 }