* [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-13 8:48 [Qemu-devel] [PATCH v8 0/4] Introduce strtosz and make use of it Jes.Sorensen
@ 2010-10-13 8:48 ` Jes.Sorensen
2010-10-13 13:28 ` Markus Armbruster
2010-10-13 8:48 ` [Qemu-devel] [PATCH 2/4] Add support for 'o' octet (bytes) format as monitor parameter Jes.Sorensen
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-13 8:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
strtosz() returns -1 on error. It now supports human unit formats in
eg. 1.0G, with better error handling.
The following suffixes are supported:
B/b = bytes
K/k = KB
M/m = MB
G/g = GB
T/t = TB
This patch changes -numa and -m input to use strtosz().
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
cutils.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-common.h | 1 +
vl.c | 31 +++++++---------------
3 files changed, 90 insertions(+), 21 deletions(-)
diff --git a/cutils.c b/cutils.c
index 5883737..4bb459d 100644
--- a/cutils.c
+++ b/cutils.c
@@ -23,6 +23,7 @@
*/
#include "qemu-common.h"
#include "host-utils.h"
+#include <math.h>
void pstrcpy(char *buf, int buf_size, const char *str)
{
@@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
}
#endif
+/*
+ * 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. Default without any postfix
+ * is MB. End pointer will be returned in *end, if not NULL.
+ * Return -1 on error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+ ssize_t retval = -1;
+ char *endptr, c;
+ int mul_required = 0;
+ double val, mul, integral, fraction;
+
+ errno = 0;
+ val = strtod(nptr, &endptr);
+ if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
+ val >= HUGE_VAL) {
+ goto fail;
+ }
+ integral = modf(val, &fraction);
+ if (integral != 0) {
+ mul_required = 1;
+ }
+ /*
+ * Any whitespace character is fine for terminating the number,
+ * in addition we accept ',' to handle strings where the size is
+ * part of a multi token argument. Otherwise check that the suffix
+ * is just one character.
+ */
+ c = *endptr++;
+ if (isspace(c) || c == '\0' || c == ',') {
+ c = 0;
+ } else if (!isspace(*endptr) && *endptr != 0) {
+ goto fail;
+ }
+ switch (c) {
+ case 'B':
+ case 'b':
+ mul = 1;
+ if (mul_required) {
+ goto fail;
+ }
+ break;
+ case 'K':
+ case 'k':
+ mul = 1 << 10;
+ break;
+ case 0:
+ if (mul_required) {
+ goto fail;
+ }
+ case 'M':
+ case 'm':
+ mul = 1ULL << 20;
+ break;
+ case 'G':
+ case 'g':
+ mul = 1ULL << 30;
+ break;
+ case 'T':
+ case 't':
+ mul = 1ULL << 40;
+ break;
+ default:
+ goto fail;
+ }
+ if (val * mul >= ~(size_t)0) {
+ goto fail;
+ }
+ retval = (val * mul);
+
+fail:
+ if (end) {
+ *end = endptr;
+ }
+
+ return retval;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
/* path.c */
void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ 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;
+ ssize_t sval;
+ sval = strtosz(option, NULL);
+ if (sval < 0) {
+ fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+ exit(1);
}
- node_mem[nodenr] = value;
+ node_mem[nodenr] = sval;
}
if (get_param_value(option, 128, "cpus", optarg) == 0) {
node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
exit(0);
break;
case QEMU_OPTION_m: {
- uint64_t value;
- char *ptr;
+ ssize_t value;
- 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 = strtosz(optarg, NULL);
+ if (value < 0) {
fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
exit(1);
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-13 8:48 ` [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count Jes.Sorensen
@ 2010-10-13 13:28 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2010-10-13 13:28 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: pbonzini, qemu-devel
Jes.Sorensen@redhat.com writes:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error. It now supports human unit formats in
> eg. 1.0G, with better error handling.
>
> The following suffixes are supported:
> B/b = bytes
> K/k = KB
> M/m = MB
> G/g = GB
> T/t = TB
>
> This patch changes -numa and -m input to use strtosz().
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> cutils.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-common.h | 1 +
> vl.c | 31 +++++++---------------
> 3 files changed, 90 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..4bb459d 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -23,6 +23,7 @@
> */
> #include "qemu-common.h"
> #include "host-utils.h"
> +#include <math.h>
>
> void pstrcpy(char *buf, int buf_size, const char *str)
> {
> @@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
> }
> #endif
>
> +/*
> + * 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. Default without any postfix
> + * is MB. End pointer will be returned in *end, if not NULL.
> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> + ssize_t retval = -1;
> + char *endptr, c;
> + int mul_required = 0;
> + double val, mul, integral, fraction;
> +
> + errno = 0;
> + val = strtod(nptr, &endptr);
> + if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
> + val >= HUGE_VAL) {
> + goto fail;
> + }
> + integral = modf(val, &fraction);
> + if (integral != 0) {
> + mul_required = 1;
> + }
> + /*
> + * Any whitespace character is fine for terminating the number,
> + * in addition we accept ',' to handle strings where the size is
> + * part of a multi token argument. Otherwise check that the suffix
> + * is just one character.
> + */
> + c = *endptr++;
Uh-oh...
> + if (isspace(c) || c == '\0' || c == ',') {
> + c = 0;
> + } else if (!isspace(*endptr) && *endptr != 0) {
> + goto fail;
> + }
> + switch (c) {
> + case 'B':
> + case 'b':
> + mul = 1;
> + if (mul_required) {
> + goto fail;
> + }
> + break;
> + case 'K':
> + case 'k':
> + mul = 1 << 10;
> + break;
> + case 0:
> + if (mul_required) {
> + goto fail;
> + }
> + case 'M':
> + case 'm':
> + mul = 1ULL << 20;
> + break;
> + case 'G':
> + case 'g':
> + mul = 1ULL << 30;
> + break;
> + case 'T':
> + case 't':
> + mul = 1ULL << 40;
> + break;
> + default:
> + goto fail;
> + }
I'm afraid strtosz("1.0x", &end) makes end point behind the 'x' instead
of to the 'x'.
Solution: increment endptr when the unit character is consumed,
i.e. after the switch. Finish consuming the unit character before
checking the suffix.
> + if (val * mul >= ~(size_t)0) {
> + goto fail;
> + }
> + retval = (val * mul);
> +
> +fail:
> + if (end) {
> + *end = endptr;
> + }
> +
> + return retval;
> +}
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/4] Add support for 'o' octet (bytes) format as monitor parameter.
2010-10-13 8:48 [Qemu-devel] [PATCH v8 0/4] Introduce strtosz and make use of it Jes.Sorensen
2010-10-13 8:48 ` [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count Jes.Sorensen
@ 2010-10-13 8:48 ` Jes.Sorensen
2010-10-13 8:48 ` [Qemu-devel] [PATCH 3/4] Switch migrate_set_speed() to take an 'o' argument rather than a float Jes.Sorensen
2010-10-13 8:48 ` [Qemu-devel] [PATCH 4/4] Remove obsolete 'f' double parameter type Jes.Sorensen
3 siblings, 0 replies; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-13 8:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
Octet format relies on strtosz which supports K/k, M/m, G/g, T/t
suffixes and unit support for humans, like 1.3G
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
monitor.c | 29 +++++++++++++++++++++++++++++
1 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/monitor.c b/monitor.c
index fbb678d..b0ea5a3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -78,6 +78,11 @@
* 'l' target long (32 or 64 bit)
* 'M' just like 'l', except in user mode the value is
* multiplied by 2^20 (think Mebibyte)
+ * 'o' octets (aka bytes)
+ * user mode accepts an optional T, t, G, g, M, m, K, k
+ * suffix, which multiplies the value by 2^40 for
+ * suffixes T and t, 2^30 for suffixes G and g, 2^20 for
+ * M and m, 2^10 for K and k
* 'f' double
* user mode accepts an optional G, g, M, m, K, k suffix,
* which multiplies the value by 2^30 for suffixes G and
@@ -3699,6 +3704,29 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
qdict_put(qdict, key, qint_from_int(val));
}
break;
+ case 'o':
+ {
+ ssize_t val;
+ char *end;
+
+ while (qemu_isspace(*p)) {
+ p++;
+ }
+ if (*typestr == '?') {
+ typestr++;
+ if (*p == '\0') {
+ break;
+ }
+ }
+ val = strtosz(p, &end);
+ if (val < 0) {
+ monitor_printf(mon, "invalid size\n");
+ goto fail;
+ }
+ qdict_put(qdict, key, qint_from_int(val));
+ p = end;
+ }
+ break;
case 'f':
case 'T':
{
@@ -4196,6 +4224,7 @@ static int check_client_args_type(const QDict *client_args,
case 'i':
case 'l':
case 'M':
+ case 'o':
if (qobject_type(client_arg) != QTYPE_QINT) {
qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
"int");
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 3/4] Switch migrate_set_speed() to take an 'o' argument rather than a float.
2010-10-13 8:48 [Qemu-devel] [PATCH v8 0/4] Introduce strtosz and make use of it Jes.Sorensen
2010-10-13 8:48 ` [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count Jes.Sorensen
2010-10-13 8:48 ` [Qemu-devel] [PATCH 2/4] Add support for 'o' octet (bytes) format as monitor parameter Jes.Sorensen
@ 2010-10-13 8:48 ` Jes.Sorensen
2010-10-13 8:48 ` [Qemu-devel] [PATCH 4/4] Remove obsolete 'f' double parameter type Jes.Sorensen
3 siblings, 0 replies; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-13 8:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
Clarify default value of MB in migration speed argument in monitor, if
no suffix is specified. This differ from previous default of bytes,
but is consistent with the rest of the places where we accept a size
argument.
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
hmp-commands.hx | 5 +++--
migration.c | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..e5585ba 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -754,9 +754,10 @@ ETEXI
{
.name = "migrate_set_speed",
- .args_type = "value:f",
+ .args_type = "value:o",
.params = "value",
- .help = "set maximum speed (in bytes) for migrations",
+ .help = "set maximum speed (in bytes) for migrations. "
+ "Defaults to MB if no size suffix is specified, ie. B/K/M/G/T",
.user_print = monitor_user_noop,
.mhandler.cmd_new = do_migrate_set_speed,
},
diff --git a/migration.c b/migration.c
index 468d517..9ee8b17 100644
--- a/migration.c
+++ b/migration.c
@@ -132,10 +132,10 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
- double d;
+ int64_t d;
FdMigrationState *s;
- d = qdict_get_double(qdict, "value");
+ d = qdict_get_int(qdict, "value");
d = MAX(0, MIN(UINT32_MAX, d));
max_throttle = d;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 4/4] Remove obsolete 'f' double parameter type
2010-10-13 8:48 [Qemu-devel] [PATCH v8 0/4] Introduce strtosz and make use of it Jes.Sorensen
` (2 preceding siblings ...)
2010-10-13 8:48 ` [Qemu-devel] [PATCH 3/4] Switch migrate_set_speed() to take an 'o' argument rather than a float Jes.Sorensen
@ 2010-10-13 8:48 ` Jes.Sorensen
3 siblings, 0 replies; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-13 8:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
'f' double is no longer used, and we should be using floating point
variables to store byte sizes. Remove it.
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
monitor.c | 18 +-----------------
1 files changed, 1 insertions(+), 17 deletions(-)
diff --git a/monitor.c b/monitor.c
index b0ea5a3..078a66e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,10 +83,6 @@
* suffix, which multiplies the value by 2^40 for
* suffixes T and t, 2^30 for suffixes G and g, 2^20 for
* M and m, 2^10 for K and k
- * 'f' double
- * user mode accepts an optional G, g, M, m, K, k suffix,
- * which multiplies the value by 2^30 for suffixes G and
- * g, 2^20 for M and m, 2^10 for K and k
* 'T' double
* user mode accepts an optional ms, us, ns suffix,
* which divides the value by 1e3, 1e6, 1e9, respectively
@@ -3727,7 +3723,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
p = end;
}
break;
- case 'f':
case 'T':
{
double val;
@@ -3743,17 +3738,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
if (get_double(mon, &val, &p) < 0) {
goto fail;
}
- if (c == 'f' && *p) {
- switch (*p) {
- case 'K': case 'k':
- val *= 1 << 10; p++; break;
- case 'M': case 'm':
- val *= 1 << 20; p++; break;
- case 'G': case 'g':
- val *= 1 << 30; p++; break;
- }
- }
- if (c == 'T' && p[0] && p[1] == 's') {
+ if (p[0] && p[1] == 's') {
switch (*p) {
case 'm':
val /= 1e3; p += 2; break;
@@ -4231,7 +4216,6 @@ static int check_client_args_type(const QDict *client_args,
return -1;
}
break;
- case 'f':
case 'T':
if (qobject_type(client_arg) != QTYPE_QINT &&
qobject_type(client_arg) != QTYPE_QFLOAT) {
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-21 15:15 [Qemu-devel] [PATCH v9 0/4] Introduce strtosz and make use of it Jes.Sorensen
@ 2010-10-21 15:15 ` Jes.Sorensen
0 siblings, 0 replies; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-21 15:15 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
strtosz() returns -1 on error. It now supports human unit formats in
eg. 1.0G, with better error handling.
The following suffixes are supported:
B/b = bytes
K/k = KB
M/m = MB
G/g = GB
T/t = TB
This patch changes -numa and -m input to use strtosz().
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
cutils.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-common.h | 1 +
vl.c | 31 ++++++-------------
3 files changed, 99 insertions(+), 21 deletions(-)
diff --git a/cutils.c b/cutils.c
index 5883737..28089aa 100644
--- a/cutils.c
+++ b/cutils.c
@@ -23,6 +23,7 @@
*/
#include "qemu-common.h"
#include "host-utils.h"
+#include <math.h>
void pstrcpy(char *buf, int buf_size, const char *str)
{
@@ -283,3 +284,90 @@ int fcntl_setfl(int fd, int flag)
}
#endif
+/*
+ * 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. Default without any postfix
+ * is MB. End pointer will be returned in *end, if not NULL. A valid
+ * value must be terminated by whitespace, ',' or '\0'. Return -1 on
+ * error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+ ssize_t retval = -1;
+ char *endptr, c;
+ int mul_required = 0;
+ double val, mul, integral, fraction;
+
+ errno = 0;
+ val = strtod(nptr, &endptr);
+ if (isnan(val) || endptr == nptr || errno != 0) {
+ goto fail;
+ }
+ integral = modf(val, &fraction);
+ if (integral != 0) {
+ mul_required = 1;
+ }
+ /*
+ * Any whitespace character is fine for terminating the number,
+ * in addition we accept ',' to handle strings where the size is
+ * part of a multi token argument.
+ */
+ c = *endptr;
+ if (isspace(c) || c == '\0' || c == ',') {
+ c = 0;
+ }
+ switch (c) {
+ case 'B':
+ case 'b':
+ mul = 1;
+ if (mul_required) {
+ goto fail;
+ }
+ break;
+ case 'K':
+ case 'k':
+ mul = 1 << 10;
+ break;
+ case 0:
+ if (mul_required) {
+ goto fail;
+ }
+ case 'M':
+ case 'm':
+ mul = 1ULL << 20;
+ break;
+ case 'G':
+ case 'g':
+ mul = 1ULL << 30;
+ break;
+ case 'T':
+ case 't':
+ mul = 1ULL << 40;
+ break;
+ default:
+ goto fail;
+ }
+ /*
+ * If not terminated by whitespace, ',', or \0, increment endptr
+ * to point to next character, then check that we are terminated
+ * by an appropriate separating character, ie. whitespace, ',', or
+ * \0. If not, we are seeing trailing garbage, thus fail.
+ */
+ if (c != 0) {
+ endptr++;
+ if (!isspace(*endptr) && *endptr != ',' && *endptr != 0) {
+ goto fail;
+ }
+ }
+ if ((val * mul >= ~(size_t)0) || val < 0) {
+ goto fail;
+ }
+ retval = val * mul;
+
+fail:
+ if (end) {
+ *end = endptr;
+ }
+
+ return retval;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
/* path.c */
void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ 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;
+ ssize_t sval;
+ sval = strtosz(option, NULL);
+ if (sval < 0) {
+ fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+ exit(1);
}
- node_mem[nodenr] = value;
+ node_mem[nodenr] = sval;
}
if (get_param_value(option, 128, "cpus", optarg) == 0) {
node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
exit(0);
break;
case QEMU_OPTION_m: {
- uint64_t value;
- char *ptr;
+ ssize_t value;
- 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 = strtosz(optarg, NULL);
+ if (value < 0) {
fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
exit(1);
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-13 7:20 [Qemu-devel] [PATCH v7 0/4] Introduce strtosz and make use of it Jes.Sorensen
@ 2010-10-13 7:20 ` Jes.Sorensen
2010-10-13 8:19 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-13 7:20 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
strtosz() returns -1 on error. It now supports human unit formats in
eg. 1.0G, with better error handling.
The following suffixes are supported:
B/b = bytes
K/k = KB
M/m = MB
G/g = GB
T/t = TB
This patch changes -numa and -m input to use strtosz().
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
cutils.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-common.h | 1 +
vl.c | 31 +++++++---------------
3 files changed, 90 insertions(+), 21 deletions(-)
diff --git a/cutils.c b/cutils.c
index 5883737..44a862a 100644
--- a/cutils.c
+++ b/cutils.c
@@ -23,6 +23,7 @@
*/
#include "qemu-common.h"
#include "host-utils.h"
+#include <math.h>
void pstrcpy(char *buf, int buf_size, const char *str)
{
@@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
}
#endif
+/*
+ * 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. Default without any postfix
+ * is MB. End pointer will be returned in *end, if not NULL.
+ * Return -1 on error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+ ssize_t retval = -1;
+ char *endptr, c;
+ int mul_required = 0;
+ double val, mul, integral, fraction;
+
+ errno = 0;
+ val = strtod(nptr, &endptr);
+ if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
+ val >= HUGE_VAL) {
+ goto fail;
+ }
+ integral = modf(val, &fraction);
+ if (integral != 0) {
+ mul_required = 1;
+ }
+ /*
+ * Any whitespace character is fine for terminating the number,
+ * in addition we accept ',' to handle strings where the size is
+ * part of a multi token argument. Otherwise check that the suffix
+ * is just one character.
+ */
+ c = *endptr++;
+ if (isspace(c) || c == '\0' || c == ',') {
+ c = 0;
+ } else if (!isspace(*endptr) && *endptr != 0) {
+ goto fail;
+ }
+ switch (c) {
+ case 'B':
+ case 'b':
+ mul = 1;
+ if (mul_required) {
+ goto fail;
+ }
+ break;
+ case 'K':
+ case 'k':
+ mul = 1 << 10;
+ break;
+ case 0:
+ if (mul_required) {
+ goto fail;
+ }
+ case 'M':
+ case 'm':
+ mul = 1ULL << 20;
+ break;
+ case 'G':
+ case 'g':
+ mul = 1ULL << 30;
+ break;
+ case 'T':
+ case 't':
+ mul = 1ULL << 40;
+ break;
+ default:
+ goto fail;
+ }
+ if (val * mul >= ~(size_t)0) {
+ goto fail;
+ }
+ retval = (val * mul);
+
+ if (end) {
+ *end = endptr;
+ }
+
+fail:
+ return retval;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
/* path.c */
void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ 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;
+ ssize_t sval;
+ sval = strtosz(option, NULL);
+ if (sval < 0) {
+ fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+ exit(1);
}
- node_mem[nodenr] = value;
+ node_mem[nodenr] = sval;
}
if (get_param_value(option, 128, "cpus", optarg) == 0) {
node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
exit(0);
break;
case QEMU_OPTION_m: {
- uint64_t value;
- char *ptr;
+ ssize_t value;
- 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 = strtosz(optarg, NULL);
+ if (value < 0) {
fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
exit(1);
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-13 7:20 ` [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count Jes.Sorensen
@ 2010-10-13 8:19 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2010-10-13 8:19 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: pbonzini, qemu-devel
Jes.Sorensen@redhat.com writes:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error. It now supports human unit formats in
> eg. 1.0G, with better error handling.
>
> The following suffixes are supported:
> B/b = bytes
> K/k = KB
> M/m = MB
> G/g = GB
> T/t = TB
>
> This patch changes -numa and -m input to use strtosz().
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> cutils.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-common.h | 1 +
> vl.c | 31 +++++++---------------
> 3 files changed, 90 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..44a862a 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -23,6 +23,7 @@
> */
> #include "qemu-common.h"
> #include "host-utils.h"
> +#include <math.h>
>
> void pstrcpy(char *buf, int buf_size, const char *str)
> {
> @@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
> }
> #endif
>
> +/*
> + * 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. Default without any postfix
> + * is MB. End pointer will be returned in *end, if not NULL.
> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> + ssize_t retval = -1;
> + char *endptr, c;
> + int mul_required = 0;
> + double val, mul, integral, fraction;
> +
> + errno = 0;
> + val = strtod(nptr, &endptr);
> + if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
> + val >= HUGE_VAL) {
> + goto fail;
> + }
> + integral = modf(val, &fraction);
> + if (integral != 0) {
> + mul_required = 1;
> + }
> + /*
> + * Any whitespace character is fine for terminating the number,
> + * in addition we accept ',' to handle strings where the size is
> + * part of a multi token argument. Otherwise check that the suffix
> + * is just one character.
> + */
> + c = *endptr++;
> + if (isspace(c) || c == '\0' || c == ',') {
> + c = 0;
> + } else if (!isspace(*endptr) && *endptr != 0) {
> + goto fail;
> + }
> + switch (c) {
> + case 'B':
> + case 'b':
> + mul = 1;
> + if (mul_required) {
> + goto fail;
> + }
> + break;
> + case 'K':
> + case 'k':
> + mul = 1 << 10;
> + break;
> + case 0:
> + if (mul_required) {
> + goto fail;
> + }
> + case 'M':
> + case 'm':
> + mul = 1ULL << 20;
> + break;
> + case 'G':
> + case 'g':
> + mul = 1ULL << 30;
> + break;
> + case 'T':
> + case 't':
> + mul = 1ULL << 40;
> + break;
> + default:
> + goto fail;
> + }
> + if (val * mul >= ~(size_t)0) {
> + goto fail;
> + }
> + retval = (val * mul);
Redundant parenthesis. Not worth a respin.
> +
> + if (end) {
> + *end = endptr;
> + }
Unlike strtol() & friends, we assign to *end only on success.
> +
> +fail:
> + return retval;
> +}
[...]
*Not* worth a respin.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-12 11:10 [Qemu-devel] [PATCH v6 0/4] Introduce strtosz and make use of it Jes.Sorensen
@ 2010-10-12 11:10 ` Jes.Sorensen
2010-10-12 15:52 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-12 11:10 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
strtosz() returns -1 on error. It now supports human unit formats in
eg. 1.0G, with better error handling.
This version lets strtod() do the actual parsing instead of relying on
strspn, as well as catches NaN input.
The following suffixes are supported:
B/b = bytes
K/k = KB
M/m = MB
G/g = GB
T/t = TB
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
cutils.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-common.h | 1 +
vl.c | 31 +++++++---------------
3 files changed, 90 insertions(+), 21 deletions(-)
diff --git a/cutils.c b/cutils.c
index 5883737..8e79d92 100644
--- a/cutils.c
+++ b/cutils.c
@@ -23,6 +23,7 @@
*/
#include "qemu-common.h"
#include "host-utils.h"
+#include <math.h>
void pstrcpy(char *buf, int buf_size, const char *str)
{
@@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
}
#endif
+/*
+ * 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. Default without any postfix
+ * is MB. End pointer will be returned in *end, if end is valid.
+ * Return -1 on error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+ ssize_t retval = -1;
+ int64_t tmpval;
+ char *endptr, c;
+ int mul_required = 0;
+ double val, mul, integral, fraction;
+
+ errno = 0;
+ val = strtod(nptr, &endptr);
+ if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
+ val == HUGE_VAL) {
+ goto fail;
+ }
+ integral = modf(val, &fraction);
+ if (integral != 0) {
+ mul_required = 1;
+ }
+ /*
+ * Any whitespace character is fine for terminating the number,
+ * otherwise check that the suffix is just one character.
+ */
+ c = *endptr++;
+ if (isspace(c) || c == '\0') {
+ c = 0;
+ } else if (!isspace(*endptr) && *endptr != 0) {
+ goto fail;
+ }
+ switch (c) {
+ case 'B':
+ case 'b':
+ mul = 1;
+ if (mul_required) {
+ goto fail;
+ }
+ break;
+ case 'K':
+ case 'k':
+ mul = 1 << 10;
+ break;
+ case 0:
+ if (mul_required) {
+ goto fail;
+ }
+ case 'M':
+ case 'm':
+ mul = 1ULL << 20;
+ break;
+ case 'G':
+ case 'g':
+ mul = 1ULL << 30;
+ break;
+ case 'T':
+ case 't':
+ mul = 1ULL << 40;
+ break;
+ default:
+ goto fail;
+ }
+ tmpval = (val * mul);
+ if (tmpval >= ~(size_t)0) {
+ goto fail;
+ }
+ retval = tmpval;
+
+ if (end) {
+ *end = endptr;
+ }
+
+fail:
+ return retval;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
/* path.c */
void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ 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;
+ ssize_t sval;
+ sval = strtosz(option, NULL);
+ if (sval < 0) {
+ fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+ exit(1);
}
- node_mem[nodenr] = value;
+ node_mem[nodenr] = sval;
}
if (get_param_value(option, 128, "cpus", optarg) == 0) {
node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
exit(0);
break;
case QEMU_OPTION_m: {
- uint64_t value;
- char *ptr;
+ ssize_t value;
- 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 = strtosz(optarg, NULL);
+ if (value < 0) {
fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
exit(1);
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-12 11:10 ` [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count Jes.Sorensen
@ 2010-10-12 15:52 ` Markus Armbruster
2010-10-13 6:47 ` Jes Sorensen
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2010-10-12 15:52 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: pbonzini, qemu-devel
Still not entirely happy, but maybe we can commit it as is, and fix it
up later.
Jes.Sorensen@redhat.com writes:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error. It now supports human unit formats in
> eg. 1.0G, with better error handling.
>
> This version lets strtod() do the actual parsing instead of relying on
> strspn, as well as catches NaN input.
This is information on what changed since v5, and as such it doesn't
belong into the commit message.
> The following suffixes are supported:
> B/b = bytes
> K/k = KB
> M/m = MB
> G/g = GB
> T/t = TB
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Would be nice if commit message documented that this affects -numa and
-m. In particular that they now accept more suffixes than before.
> ---
> cutils.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-common.h | 1 +
> vl.c | 31 +++++++---------------
> 3 files changed, 90 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..8e79d92 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -23,6 +23,7 @@
> */
> #include "qemu-common.h"
> #include "host-utils.h"
> +#include <math.h>
>
> void pstrcpy(char *buf, int buf_size, const char *str)
> {
> @@ -283,3 +284,81 @@ int fcntl_setfl(int fd, int flag)
> }
> #endif
>
> +/*
> + * 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. Default without any postfix
> + * is MB. End pointer will be returned in *end, if end is valid.
Nitpick: There are plenty of invalid pointers we'll happily attempt to
use. "unless end is null" would be more precise.
> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> + ssize_t retval = -1;
> + int64_t tmpval;
> + char *endptr, c;
> + int mul_required = 0;
> + double val, mul, integral, fraction;
> +
> + errno = 0;
> + val = strtod(nptr, &endptr);
> + if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
> + val == HUGE_VAL) {
ISO C permits implementations supporting infinities to make HUGE_VAL
*not* +inf. So this may not catch +inf. val >= HUGE_VAL would.
But since we have to catch val * mul out of range further down anyway,
the check for HUGE_VAL may be redundant here.
> + goto fail;
> + }
> + integral = modf(val, &fraction);
> + if (integral != 0) {
> + mul_required = 1;
> + }
> + /*
> + * Any whitespace character is fine for terminating the number,
> + * otherwise check that the suffix is just one character.
> + */
> + c = *endptr++;
> + if (isspace(c) || c == '\0') {
> + c = 0;
> + } else if (!isspace(*endptr) && *endptr != 0) {
> + goto fail;
> + }
I'm not happy with this check.
If the caller needs a complete string consumed, then this check is
insufficient, because it doesn't catch trailing garbage as long as it
starts with whitespace. The caller still needs to check !*endptr.
If the caller needs to continue parsing after the value, and expects
anything but whitespace there, it has to copy the value first. Only
easy if the value is followed by some delimiter that can't occur in the
value. Example: parse a size value from something of them form
name=value,name=value... Need to copy up to the next comma or end of
string.
The check complicates the second case without really helping the first
case.
Nevertheless, it's good enough for the uses in this patch series, so I'm
not insisting on getting this changed now.
> + switch (c) {
> + case 'B':
> + case 'b':
> + mul = 1;
> + if (mul_required) {
> + goto fail;
> + }
> + break;
> + case 'K':
> + case 'k':
> + mul = 1 << 10;
> + break;
> + case 0:
> + if (mul_required) {
> + goto fail;
> + }
> + case 'M':
> + case 'm':
> + mul = 1ULL << 20;
> + break;
> + case 'G':
> + case 'g':
> + mul = 1ULL << 30;
> + break;
> + case 'T':
> + case 't':
> + mul = 1ULL << 40;
> + break;
> + default:
> + goto fail;
> + }
> + tmpval = (val * mul);
> + if (tmpval >= ~(size_t)0) {
> + goto fail;
val * mul may exceed the range of int64_t tmpval, and then the
assignment has undefined behavior. Obvious way to avoid that:
if (val * mul >= ~(size_t)0) {
goto fail;
}
retval = val * mul;
> + }
> + retval = tmpval;
> +
> + if (end) {
> + *end = endptr;
> + }
> +
> +fail:
> + return retval;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 81aafa0..0a062d4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
> int qemu_fls(int i);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> +ssize_t strtosz(const char *nptr, char **end);
>
> /* path.c */
> void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index df414ef..6043fa2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,16 +734,13 @@ 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;
> + ssize_t sval;
> + sval = strtosz(option, NULL);
> + if (sval < 0) {
> + fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> + exit(1);
> }
> - node_mem[nodenr] = value;
> + node_mem[nodenr] = sval;
> }
> if (get_param_value(option, 128, "cpus", optarg) == 0) {
> node_cpumask[nodenr] = 0;
> @@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
> exit(0);
> break;
> case QEMU_OPTION_m: {
> - uint64_t value;
> - char *ptr;
> + ssize_t value;
>
> - 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 = strtosz(optarg, NULL);
> + if (value < 0) {
> fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
> exit(1);
> }
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-12 15:52 ` Markus Armbruster
@ 2010-10-13 6:47 ` Jes Sorensen
2010-10-13 8:07 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Jes Sorensen @ 2010-10-13 6:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pbonzini, qemu-devel
On 10/12/10 17:52, Markus Armbruster wrote:
> Still not entirely happy, but maybe we can commit it as is, and fix it
> up later.
No worries, I think this is the most serious review I have ever received
for any piece of code, but you're finding valid points so it's good. If
all of QEMU had been reviewed like this we would be in really good shape :)
>> The following suffixes are supported:
>> B/b = bytes
>> K/k = KB
>> M/m = MB
>> G/g = GB
>> T/t = TB
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Would be nice if commit message documented that this affects -numa and
> -m. In particular that they now accept more suffixes than before.
Will address this in the commit message.
>> +/*
>> + * 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. Default without any postfix
>> + * is MB. End pointer will be returned in *end, if end is valid.
>
> Nitpick: There are plenty of invalid pointers we'll happily attempt to
> use. "unless end is null" would be more precise.
Fixed
>> + errno = 0;
>> + val = strtod(nptr, &endptr);
>> + if (isnan(val) || endptr == nptr || errno != 0 || val < 0 ||
>> + val == HUGE_VAL) {
>
> ISO C permits implementations supporting infinities to make HUGE_VAL
> *not* +inf. So this may not catch +inf. val >= HUGE_VAL would.
>
> But since we have to catch val * mul out of range further down anyway,
> the check for HUGE_VAL may be redundant here.
Valid point, fixed in the upcoming version.
>> + c = *endptr++;
>> + if (isspace(c) || c == '\0') {
>> + c = 0;
>> + } else if (!isspace(*endptr) && *endptr != 0) {
>> + goto fail;
>> + }
>
> I'm not happy with this check.
>
> If the caller needs a complete string consumed, then this check is
> insufficient, because it doesn't catch trailing garbage as long as it
> starts with whitespace. The caller still needs to check !*endptr.
>
> If the caller needs to continue parsing after the value, and expects
> anything but whitespace there, it has to copy the value first. Only
> easy if the value is followed by some delimiter that can't occur in the
> value. Example: parse a size value from something of them form
> name=value,name=value... Need to copy up to the next comma or end of
> string.
>
> The check complicates the second case without really helping the first
> case.
>
> Nevertheless, it's good enough for the uses in this patch series, so I'm
> not insisting on getting this changed now.
I hadn't thought of case #2, but I think that is pretty easy to handle
by accepting ',' as a separator as well. It's worth keeping in kind that
the old code didn't do anything with trailing garbage either, it was
silently ignored.
For case #1 then I think it's ok to just accept trailing garbage, the
old code would simply use strtoull and leave it at that.
>> + tmpval = (val * mul);
>> + if (tmpval >= ~(size_t)0) {
>> + goto fail;
>
> val * mul may exceed the range of int64_t tmpval, and then the
> assignment has undefined behavior. Obvious way to avoid that:
>
> if (val * mul >= ~(size_t)0) {
> goto fail;
> }
> retval = val * mul;
Good point, fixed.
Updated version coming up shortly.
Jes
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-13 6:47 ` Jes Sorensen
@ 2010-10-13 8:07 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2010-10-13 8:07 UTC (permalink / raw)
To: Jes Sorensen; +Cc: pbonzini, qemu-devel
Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> On 10/12/10 17:52, Markus Armbruster wrote:
[...]
>>> + c = *endptr++;
>>> + if (isspace(c) || c == '\0') {
>>> + c = 0;
>>> + } else if (!isspace(*endptr) && *endptr != 0) {
>>> + goto fail;
>>> + }
>>
>> I'm not happy with this check.
>>
>> If the caller needs a complete string consumed, then this check is
>> insufficient, because it doesn't catch trailing garbage as long as it
>> starts with whitespace. The caller still needs to check !*endptr.
>>
>> If the caller needs to continue parsing after the value, and expects
>> anything but whitespace there, it has to copy the value first. Only
>> easy if the value is followed by some delimiter that can't occur in the
>> value. Example: parse a size value from something of them form
>> name=value,name=value... Need to copy up to the next comma or end of
>> string.
>>
>> The check complicates the second case without really helping the first
>> case.
>>
>> Nevertheless, it's good enough for the uses in this patch series, so I'm
>> not insisting on getting this changed now.
>
> I hadn't thought of case #2, but I think that is pretty easy to handle
> by accepting ',' as a separator as well.
Then callers that don't want ',' have to check the suffix.
And when a caller comes around that wants '"', it has to copy the value
string, or hack strtosz() and figure out which of the existing callers
now have to check the suffix.
strtosz() shouldn't second guess what characters can legitimately follow
(the follow set in parsing parlance). That's only known up the call
chain.
I still think there are two clean interfaces:
* Consume the longest prefix that makes sense, then stop. Return where
we stopped. Leave parsing / rejecting the suffix to the caller.
That's how strtol() & friends work.
* Consume a complete string, fail if there's trailing garbage. This is
easier to use when you want to parse complete strings. It's awkward
when you don't.
> It's worth keeping in kind that
> the old code didn't do anything with trailing garbage either, it was
> silently ignored.
>
> For case #1 then I think it's ok to just accept trailing garbage, the
> old code would simply use strtoull and leave it at that.
As long as your patch doesn't make things worse, it can be committed.
When I come across another use of strtosz() where it doesn't work
nicely, I can fix it up.
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-11 12:54 [Qemu-devel] [PATCH v5 0/4] Introduce strtosz and make use of it Jes.Sorensen
@ 2010-10-11 12:54 ` Jes.Sorensen
2010-10-11 16:42 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Jes.Sorensen @ 2010-10-11 12:54 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, armbru
From: Jes Sorensen <Jes.Sorensen@redhat.com>
strtosz() returns -1 on error. It now supports human unit formats in
eg. 1.0G, with better error handling.
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
cutils.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-common.h | 1 +
vl.c | 31 +++++++++-------------------
3 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/cutils.c b/cutils.c
index 5883737..e5a135e 100644
--- a/cutils.c
+++ b/cutils.c
@@ -283,3 +283,64 @@ int fcntl_setfl(int fd, int flag)
}
#endif
+/*
+ * Convert string to bytes, allowing either K/k for KB, M/m for MB,
+ * G/g 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 -1 on error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+ ssize_t retval = -1;
+ int64_t tmpval;
+ char *endptr;
+ int mul_required = 0;
+ double val, mul = 1;
+
+ endptr = (char *)nptr + strspn(nptr, " 0123456789");
+ if (*endptr == '.') {
+ mul_required = 1;
+ }
+
+ errno = 0;
+ val = strtod(nptr, &endptr);
+ if (endptr == nptr || errno != 0 || val < 0)
+ goto fail;
+
+ switch (*endptr++) {
+ case 'K':
+ case 'k':
+ mul = 1 << 10;
+ break;
+ case 0:
+ case ' ':
+ if (mul_required) {
+ goto fail;
+ }
+ case 'M':
+ case 'm':
+ mul = 1ULL << 20;
+ break;
+ case 'G':
+ case 'g':
+ mul = 1ULL << 30;
+ break;
+ case 'T':
+ case 't':
+ mul = 1ULL << 40;
+ break;
+ default:
+ goto fail;
+ }
+
+ tmpval = (val * mul);
+ if (tmpval >= ~(size_t)0)
+ goto fail;
+ retval = tmpval;
+
+ if (end)
+ *end = endptr;
+
+fail:
+ return retval;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
/* path.c */
void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ 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;
+ ssize_t sval;
+ sval = strtosz(option, NULL);
+ if (sval < 0) {
+ fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+ exit(1);
}
- node_mem[nodenr] = value;
+ node_mem[nodenr] = sval;
}
if (get_param_value(option, 128, "cpus", optarg) == 0) {
node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
exit(0);
break;
case QEMU_OPTION_m: {
- uint64_t value;
- char *ptr;
+ ssize_t value;
- 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 = strtosz(optarg, NULL);
+ if (value < 0) {
fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
exit(1);
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count.
2010-10-11 12:54 ` [Qemu-devel] [PATCH 1/4] Introduce strtosz() library function to convert a string to a byte count Jes.Sorensen
@ 2010-10-11 16:42 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2010-10-11 16:42 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: pbonzini, qemu-devel
Jes, I feel a bit bad finding still more faults, but here goes...
Jes.Sorensen@redhat.com writes:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error. It now supports human unit formats in
> eg. 1.0G, with better error handling.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> cutils.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-common.h | 1 +
> vl.c | 31 +++++++++-------------------
> 3 files changed, 72 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..e5a135e 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -283,3 +283,64 @@ int fcntl_setfl(int fd, int flag)
> }
> #endif
>
> +/*
> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
> + * G/g 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 -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> + ssize_t retval = -1;
> + int64_t tmpval;
> + char *endptr;
> + int mul_required = 0;
> + double val, mul = 1;
> +
> + endptr = (char *)nptr + strspn(nptr, " 0123456789");
> + if (*endptr == '.') {
> + mul_required = 1;
> + }
> +
> + errno = 0;
> + val = strtod(nptr, &endptr);
> + if (endptr == nptr || errno != 0 || val < 0)
> + goto fail;
Consider strtosz("\t1.5", &eptr): Your strspn() returns 0, endptr points
to tab, and mul_required remains false. strtod() succeeds and returns
1.5. Oops.
strtod() skips whitespace, i.e. " \f\n\r\t\v" (assuming C locale). You
could strspn() those.
But I'd leave the parsing to strtod(), and simply test whether the final
value is an integer. See below for an obvious way to do that.
> +
> + switch (*endptr++) {
> + case 'K':
> + case 'k':
> + mul = 1 << 10;
> + break;
> + case 0:
> + case ' ':
> + if (mul_required) {
> + goto fail;
> + }
> + case 'M':
> + case 'm':
> + mul = 1ULL << 20;
> + break;
> + case 'G':
> + case 'g':
> + mul = 1ULL << 30;
> + break;
> + case 'T':
> + case 't':
> + mul = 1ULL << 40;
> + break;
> + default:
> + goto fail;
If there's no unit character, the stop character must be either 0 or
' '. Why only space, but not other whitespace characters, such as tab?
If there is a unit character, any stop character is fine.
I think there are two clean interfaces:
* Consume the longest prefix that makes sense, then stop. Return where
we stopped. Leave parsing / rejecting the suffix to the caller.
* Consume a complete string, fail if there's trailing garbage. This is
easier to use when you want to parse complete strings. It's awkward
when you don't.
> + }
> +
> + tmpval = (val * mul);
> + if (tmpval >= ~(size_t)0)
> + goto fail;
strtod() returns HUGE_VAL on overflow. Converting that to int64_t has
undefined behavior. Clean code has to do something like
val *= mul;
if (val < 0 || val > ~(size_t)0 || val != val) {
goto fail;
}
The val != val part cares for the pathological case when some joker
passes us "NAN".
The check for integer promised above:
if ((int64_t)val != val) {
goto fail;
}
Depends on val *= mul, of course.
> + retval = tmpval;
> +
> + if (end)
> + *end = endptr;
> +
> +fail:
> + return retval;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 81aafa0..0a062d4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
> int qemu_fls(int i);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> +ssize_t strtosz(const char *nptr, char **end);
>
> /* path.c */
> void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index df414ef..6043fa2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,16 +734,13 @@ 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;
> + ssize_t sval;
> + sval = strtosz(option, NULL);
> + if (sval < 0) {
> + fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> + exit(1);
Still ignores trailing garbage. No need to tighten that now.
> }
> - node_mem[nodenr] = value;
> + node_mem[nodenr] = sval;
> }
> if (get_param_value(option, 128, "cpus", optarg) == 0) {
> node_cpumask[nodenr] = 0;
> @@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
> exit(0);
> break;
> case QEMU_OPTION_m: {
> - uint64_t value;
> - char *ptr;
> + ssize_t value;
>
> - 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 = strtosz(optarg, NULL);
> + if (value < 0) {
> fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
> exit(1);
> }
Same here.
^ permalink raw reply [flat|nested] 15+ messages in thread