[Qemu-devel] [PATCH v2 1/4] utils: rename strtosz to use qemu prefix

[Qemu-devel] [PATCH v2 1/4] utils: rename strtosz to use qemu prefix

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Not only it makes sense, but it gets rid of checkpatch warning:
WARNING: consider using qemu_strtosz in preference to strtosz

Also remove the tabs to please checkpatch.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qemu-common.h | 27 ++++++++++++++-------------
 monitor.c             |  2 +-
 qapi/opts-visitor.c   |  4 ++--
 qemu-img.c            |  5 +++--
 qemu-io-cmds.c        |  2 +-
 target-i386/cpu.c     |  4 ++--
 util/cutils.c         | 25 +++++++++++++------------
 7 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 01d29dd..0bd212b 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -217,22 +217,23 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
 int parse_uint_full(const char *s, unsigned long long *value, int base);
 
 /*
- * strtosz() suffixes used to specify the default treatment of an
- * argument passed to strtosz() without an explicit suffix.
+ * qemu_strtosz() suffixes used to specify the default treatment of an
+ * argument passed to qemu_strtosz() without an explicit suffix.
  * These should be defined using upper case characters in the range
- * A-Z, as strtosz() will use qemu_toupper() on the given argument
+ * A-Z, as qemu_strtosz() will use qemu_toupper() on the given argument
  * prior to comparison.
  */
-#define STRTOSZ_DEFSUFFIX_EB	'E'
-#define STRTOSZ_DEFSUFFIX_PB	'P'
-#define STRTOSZ_DEFSUFFIX_TB	'T'
-#define STRTOSZ_DEFSUFFIX_GB	'G'
-#define STRTOSZ_DEFSUFFIX_MB	'M'
-#define STRTOSZ_DEFSUFFIX_KB	'K'
-#define STRTOSZ_DEFSUFFIX_B	'B'
-int64_t strtosz(const char *nptr, char **end);
-int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
-int64_t strtosz_suffix_unit(const char *nptr, char **end,
+#define QEMU_STRTOSZ_DEFSUFFIX_EB 'E'
+#define QEMU_STRTOSZ_DEFSUFFIX_PB 'P'
+#define QEMU_STRTOSZ_DEFSUFFIX_TB 'T'
+#define QEMU_STRTOSZ_DEFSUFFIX_GB 'G'
+#define QEMU_STRTOSZ_DEFSUFFIX_MB 'M'
+#define QEMU_STRTOSZ_DEFSUFFIX_KB 'K'
+#define QEMU_STRTOSZ_DEFSUFFIX_B 'B'
+int64_t qemu_strtosz(const char *nptr, char **end);
+int64_t qemu_strtosz_suffix(const char *nptr, char **end,
+                            const char default_suffix);
+int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit);
 #define K_BYTE     (1ULL << 10)
 #define M_BYTE     (1ULL << 20)
diff --git a/monitor.c b/monitor.c
index 1f43263..005d3d9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2681,7 +2681,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
                         break;
                     }
                 }
-                val = strtosz(p, &end);
+                val = qemu_strtosz(p, &end);
                 if (val < 0) {
                     monitor_printf(mon, "invalid size\n");
                     goto fail;
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 7ae33b3..cd10392 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -474,8 +474,8 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
         return;
     }
 
-    val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
-                         STRTOSZ_DEFSUFFIX_B);
+    val = qemu_strtosz_suffix(opt->str ? opt->str : "", &endptr,
+                         QEMU_STRTOSZ_DEFSUFFIX_B);
     if (val < 0 || *endptr) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
                    "a size value representible as a non-negative int64");
diff --git a/qemu-img.c b/qemu-img.c
index 6ff4e85..7d65c0a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -338,7 +338,8 @@ static int img_create(int argc, char **argv)
     if (optind < argc) {
         int64_t sval;
         char *end;
-        sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
+        sval = qemu_strtosz_suffix(argv[optind++], &end,
+                                   QEMU_STRTOSZ_DEFSUFFIX_B);
         if (sval < 0 || *end) {
             if (sval == -ERANGE) {
                 error_report("Image size must be less than 8 EiB!");
@@ -1607,7 +1608,7 @@ static int img_convert(int argc, char **argv)
         {
             int64_t sval;
             char *end;
-            sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);
+            sval = qemu_strtosz_suffix(optarg, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
             if (sval < 0 || *end) {
                 error_report("Invalid minimum zero buffer size for sparse output specified");
                 ret = -1;
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index d6572a8..6e5d1e4 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -136,7 +136,7 @@ static char **breakline(char *input, int *count)
 static int64_t cvtnum(const char *s)
 {
     char *end;
-    return strtosz_suffix(s, &end, STRTOSZ_DEFSUFFIX_B);
+    return qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
 }
 
 #define EXABYTES(x)     ((long long)(x) << 60)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7c52714..d2b6bc5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1893,8 +1893,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                 char *err;
                 char num[32];
 
-                tsc_freq = strtosz_suffix_unit(val, &err,
-                                               STRTOSZ_DEFSUFFIX_B, 1000);
+                tsc_freq = qemu_strtosz_suffix_unit(val, &err,
+                                               QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
                 if (tsc_freq < 0 || *err) {
                     error_setg(errp, "bad numerical value %s", val);
                     return;
diff --git a/util/cutils.c b/util/cutils.c
index ae35198..cfeb848 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -276,19 +276,19 @@ int fcntl_setfl(int fd, int flag)
 static int64_t suffix_mul(char suffix, int64_t unit)
 {
     switch (qemu_toupper(suffix)) {
-    case STRTOSZ_DEFSUFFIX_B:
+    case QEMU_STRTOSZ_DEFSUFFIX_B:
         return 1;
-    case STRTOSZ_DEFSUFFIX_KB:
+    case QEMU_STRTOSZ_DEFSUFFIX_KB:
         return unit;
-    case STRTOSZ_DEFSUFFIX_MB:
+    case QEMU_STRTOSZ_DEFSUFFIX_MB:
         return unit * unit;
-    case STRTOSZ_DEFSUFFIX_GB:
+    case QEMU_STRTOSZ_DEFSUFFIX_GB:
         return unit * unit * unit;
-    case STRTOSZ_DEFSUFFIX_TB:
+    case QEMU_STRTOSZ_DEFSUFFIX_TB:
         return unit * unit * unit * unit;
-    case STRTOSZ_DEFSUFFIX_PB:
+    case QEMU_STRTOSZ_DEFSUFFIX_PB:
         return unit * unit * unit * unit * unit;
-    case STRTOSZ_DEFSUFFIX_EB:
+    case QEMU_STRTOSZ_DEFSUFFIX_EB:
         return unit * unit * unit * unit * unit * unit;
     }
     return -1;
@@ -300,7 +300,7 @@ static int64_t suffix_mul(char suffix, int64_t unit)
  * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
  * other error.
  */
-int64_t strtosz_suffix_unit(const char *nptr, char **end,
+int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit)
 {
     int64_t retval = -EINVAL;
@@ -343,14 +343,15 @@ fail:
     return retval;
 }
 
-int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
+int64_t qemu_strtosz_suffix(const char *nptr, char **end,
+                            const char default_suffix)
 {
-    return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
+    return qemu_strtosz_suffix_unit(nptr, end, default_suffix, 1024);
 }
 
-int64_t strtosz(const char *nptr, char **end)
+int64_t qemu_strtosz(const char *nptr, char **end)
 {
-    return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
+    return qemu_strtosz_suffix(nptr, end, QEMU_STRTOSZ_DEFSUFFIX_MB);
 }
 
 /**
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/4] tests: add some qemu_strtosz() tests

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

While reading the function I decided to write some tests.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-cutils.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 0046c61..a3de6ab 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1352,6 +1352,86 @@ static void test_qemu_strtoull_full_max(void)
     g_assert_cmpint(res, ==, ULLONG_MAX);
 }
 
+static void test_qemu_strtosz_simple(void)
+{
+    const char *str = "12345M";
+    char *endptr = NULL;
+    int64_t res;
+
+    res = qemu_strtosz(str, &endptr);
+    g_assert_cmpint(res, ==, 12345 * M_BYTE);
+    g_assert(endptr == str + 6);
+
+    res = qemu_strtosz(str, NULL);
+    g_assert_cmpint(res, ==, 12345 * M_BYTE);
+}
+
+static void test_qemu_strtosz_units(void)
+{
+    const char *none = "1";
+    const char *b = "1B";
+    const char *k = "1K";
+    const char *m = "1M";
+    const char *g = "1G";
+    const char *t = "1T";
+    const char *p = "1P";
+    const char *e = "1E";
+    int64_t res;
+
+    /* default is M */
+    res = qemu_strtosz(none, NULL);
+    g_assert_cmpint(res, ==, M_BYTE);
+
+    res = qemu_strtosz(b, NULL);
+    g_assert_cmpint(res, ==, 1);
+
+    res = qemu_strtosz(k, NULL);
+    g_assert_cmpint(res, ==, K_BYTE);
+
+    res = qemu_strtosz(m, NULL);
+    g_assert_cmpint(res, ==, M_BYTE);
+
+    res = qemu_strtosz(g, NULL);
+    g_assert_cmpint(res, ==, G_BYTE);
+
+    res = qemu_strtosz(t, NULL);
+    g_assert_cmpint(res, ==, T_BYTE);
+
+    res = qemu_strtosz(p, NULL);
+    g_assert_cmpint(res, ==, P_BYTE);
+
+    res = qemu_strtosz(e, NULL);
+    g_assert_cmpint(res, ==, E_BYTE);
+}
+
+static void test_qemu_strtosz_float(void)
+{
+    const char *str = "12.345M";
+    int64_t res;
+
+    res = qemu_strtosz(str, NULL);
+    g_assert_cmpint(res, ==, 12.345 * M_BYTE);
+}
+
+static void test_qemu_strtosz_erange(void)
+{
+    const char *str = "10E";
+    int64_t res;
+
+    res = qemu_strtosz(str, NULL);
+    g_assert_cmpint(res, ==, -ERANGE);
+}
+
+static void test_qemu_strtosz_suffix_unit(void)
+{
+    const char *str = "12345";
+    int64_t res;
+
+    res = qemu_strtosz_suffix_unit(str, NULL,
+                                   QEMU_STRTOSZ_DEFSUFFIX_KB, 1000);
+    g_assert_cmpint(res, ==, 12345000);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -1502,5 +1582,16 @@ int main(int argc, char **argv)
     g_test_add_func("/cutils/qemu_strtoull_full/max",
                     test_qemu_strtoull_full_max);
 
+    g_test_add_func("/cutils/strtosz/simple",
+                    test_qemu_strtosz_simple);
+    g_test_add_func("/cutils/strtosz/units",
+                    test_qemu_strtosz_units);
+    g_test_add_func("/cutils/strtosz/float",
+                    test_qemu_strtosz_float);
+    g_test_add_func("/cutils/strtosz/erange",
+                    test_qemu_strtosz_erange);
+    g_test_add_func("/cutils/strtosz/suffix-unit",
+                    test_qemu_strtosz_suffix_unit);
+
     return g_test_run();
 }
-- 
2.4.3

[Qemu-devel] [PATCH v2 3/4] checkpatch: recommend strtok_r

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

If anything, it should recommend strtok_r!

(strtok_r() is reentrant-safe and thread-safe)

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[Better matching and error message - Marc-André]
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/checkpatch.pl | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 574334b..20406d4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2463,9 +2463,13 @@ sub process {
 			WARN("__func__ should be used instead of gcc specific __FUNCTION__\n"  . $herecurr);
 		}
 
+# recommend strtok_r over strtok
+		if ($line =~ /strtok_r\(/) {
+		} elsif ($line =~ /strtok\(/) {
+                    WARN("consider using strtok_r in preference to $1\n" . $herecurr);
 # recommend qemu_strto* over strto*
-		if ($line =~ /\b(strto.*?)\s*\(/) {
-			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
+		} elsif ($line =~ /\b(strto.*?)\s*\(/) {
+                    WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
 		}
 # check for module_init(), use category-specific init macros explicitly please
 		if ($line =~ /^module_init\s*\(/) {
-- 
2.4.3

[Qemu-devel] [PATCH v2 4/4] Replace strotok() by strtok_r()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/archipelago.c | 10 +++++-----
 block/sheepdog.c    |  5 +++--
 qom/cpu.c           | 12 ++++++------
 target-i386/cpu.c   |  6 +++---
 target-sparc/cpu.c  | 10 +++++-----
 vl.c                |  6 +++---
 6 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/block/archipelago.c b/block/archipelago.c
index 855655c..1a8fcc0 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -354,17 +354,17 @@ static void parse_filename_opts(const char *filename, Error **errp,
                                 xport *mport, xport *vport)
 {
     const char *start;
-    char *tokens[4], *ds;
+    char *tokens[4], *ds, *save;
     int idx;
     xport lmport = NoPort, lvport = NoPort;
 
     strstart(filename, "archipelago:", &start);
 
     ds = g_strdup(start);
-    tokens[0] = strtok(ds, "/");
-    tokens[1] = strtok(NULL, ":");
-    tokens[2] = strtok(NULL, ":");
-    tokens[3] = strtok(NULL, "\0");
+    tokens[0] = strtok_r(ds, "/", &save);
+    tokens[1] = strtok_r(NULL, ":", &save);
+    tokens[2] = strtok_r(NULL, ":", &save);
+    tokens[3] = strtok_r(NULL, "\0", &save);
 
     if (!strlen(tokens[0])) {
         error_setg(errp, "volume name must be specified first");
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 67ca788..272a4ca 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1614,12 +1614,13 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
 {
     struct SheepdogInode *inode = &s->inode;
     const char *n1, *n2;
+    char *save;
     long copy, parity;
     char p[10];
 
     pstrcpy(p, sizeof(p), opt);
-    n1 = strtok(p, ":");
-    n2 = strtok(NULL, ":");
+    n1 = strtok_r(p, ":", &save);
+    n2 = strtok_r(NULL, ":", &save);
 
     if (!n1) {
         return -EINVAL;
diff --git a/qom/cpu.c b/qom/cpu.c
index fb80d13a..9511f10 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -42,14 +42,14 @@ bool cpu_exists(int64_t id)
 
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
 {
-    char *str, *name, *featurestr;
+    char *str, *name, *featurestr, *save;
     CPUState *cpu;
     ObjectClass *oc;
     CPUClass *cc;
     Error *err = NULL;
 
     str = g_strdup(cpu_model);
-    name = strtok(str, ",");
+    name = strtok_r(str, ",", &save);
 
     oc = cpu_class_by_name(typename, name);
     if (oc == NULL) {
@@ -60,7 +60,7 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
     cpu = CPU(object_new(object_class_get_name(oc)));
     cc = CPU_GET_CLASS(cpu);
 
-    featurestr = strtok(NULL, ",");
+    featurestr = strtok_r(NULL, ",", &save);
     cc->parse_features(cpu, featurestr, &err);
     g_free(str);
     if (err != NULL) {
@@ -276,10 +276,10 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
                                       Error **errp)
 {
     char *featurestr; /* Single "key=value" string being parsed */
-    char *val;
+    char *val, *save;
     Error *err = NULL;
 
-    featurestr = features ? strtok(features, ",") : NULL;
+    featurestr = features ? strtok_r(features, ",", &save) : NULL;
 
     while (featurestr) {
         val = strchr(featurestr, '=');
@@ -296,7 +296,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
                        featurestr);
             return;
         }
-        featurestr = strtok(NULL, ",");
+        featurestr = strtok_r(NULL, ",", &save);
     }
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d2b6bc5..2487641 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1851,7 +1851,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                                      Error **errp)
 {
     X86CPU *cpu = X86_CPU(cs);
-    char *featurestr; /* Single 'key=value" string being parsed */
+    char *save, *featurestr; /* Single 'key=value" string being parsed */
     FeatureWord w;
     /* Features to be added */
     FeatureWordArray plus_features = { 0 };
@@ -1861,7 +1861,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
 
-    featurestr = features ? strtok(features, ",") : NULL;
+    featurestr = features ? strtok_r(features, ",", &save) : NULL;
 
     while (featurestr) {
         char *val;
@@ -1930,7 +1930,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
             error_propagate(errp, local_err);
             return;
         }
-        featurestr = strtok(NULL, ",");
+        featurestr = strtok_r(NULL, ",", &save);
     }
 
     if (cpu->host_features) {
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 9528e3a..6269091 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -95,7 +95,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
     CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUSPARCState *env = &cpu->env;
     char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
+    char *featurestr, *name = strtok_r(s, ",", &save);
     sparc_def_t def1, *def = &def1;
     Error *err = NULL;
 
@@ -107,7 +107,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
     env->def = g_new0(sparc_def_t, 1);
     memcpy(env->def, def, sizeof(*def));
 
-    featurestr = strtok(NULL, ",");
+    featurestr = strtok_r(NULL, ",", &save);
     cc->parse_features(CPU(cpu), featurestr, &err);
     g_free(s);
     if (err) {
@@ -560,13 +560,13 @@ static void sparc_cpu_parse_features(CPUState *cs, char *features,
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     sparc_def_t *cpu_def = cpu->env.def;
-    char *featurestr;
+    char *featurestr, *save;
     uint32_t plus_features = 0;
     uint32_t minus_features = 0;
     uint64_t iu_version;
     uint32_t fpu_version, mmu_version, nwindows;
 
-    featurestr = features ? strtok(features, ",") : NULL;
+    featurestr = features ? strtok_r(features, ",", &save) : NULL;
     while (featurestr) {
         char *val;
 
@@ -634,7 +634,7 @@ static void sparc_cpu_parse_features(CPUState *cs, char *features,
                              "(+feature|-feature|feature=xyz)", featurestr);
             return;
         }
-        featurestr = strtok(NULL, ",");
+        featurestr = strtok_r(NULL, ",", &save);
     }
     cpu_def->features |= plus_features;
     cpu_def->features &= ~minus_features;
diff --git a/vl.c b/vl.c
index 3c6480d..bab2696 100644
--- a/vl.c
+++ b/vl.c
@@ -1323,16 +1323,16 @@ static int add_semihosting_arg(void *opaque,
 /* Use strings passed via -kernel/-append to initialize semihosting.argv[] */
 static inline void semihosting_arg_fallback(const char *file, const char *cmd)
 {
-    char *cmd_token;
+    char *cmd_token, *save;
 
     /* argv[0] */
     add_semihosting_arg(&semihosting, "arg", file, NULL);
 
     /* split -append and initialize argv[1..n] */
-    cmd_token = strtok(g_strdup(cmd), " ");
+    cmd_token = strtok_r(g_strdup(cmd), " ", &save);
     while (cmd_token) {
         add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
-        cmd_token = strtok(NULL, " ");
+        cmd_token = strtok_r(NULL, " ", &save);
     }
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 1/4] utils: rename strtosz to use qemu prefix

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

On 09/18/2015 04:59 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 

No cover letter? No description of what changed since v1? It's generally
nice to include a 0/4 letter when sending a series; with new enough git,
'git config format.coverLetter auto' does what you want.

> Not only it makes sense, but it gets rid of checkpatch warning:
> WARNING: consider using qemu_strtosz in preference to strtosz
> 
> Also remove the tabs to please checkpatch.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Fortunately, this still stands.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] tests: add some qemu_strtosz() tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On 09/18/2015 04:59 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> While reading the function I decided to write some tests.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-cutils.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)

I accepted v1 because it was better than no tests at all, but did make
some suggestions for additional tests to perform.  I'm surprised you
didn't include any of those suggestions in v2.  For example, it would be
nice if the testsuite documents a contract on what happens with a bogus
suffix: is "1234x" outright rejected, or does it parse as "1234" leaving
the pointer at 'x'?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] tests: add some qemu_strtosz() tests

[Marc-André Lureau]

Hi

----- Original Message -----
> On 09/18/2015 04:59 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > While reading the function I decided to write some tests.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  tests/test-cutils.c | 91
> >  +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> 
> I accepted v1 because it was better than no tests at all, but did make
> some suggestions for additional tests to perform.  I'm surprised you
> didn't include any of those suggestions in v2.  For example, it would be
> nice if the testsuite documents a contract on what happens with a bogus
> suffix: is "1234x" outright rejected, or does it parse as "1234" leaving
> the pointer at 'x'?

I thought you were ok with this patch as is. But I can add some failing tests if you want (although it feels like testing strtod() at this point.

Re: [Qemu-devel] [PATCH v2 2/4] tests: add some qemu_strtosz() tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

On 09/18/2015 09:10 AM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 09/18/2015 04:59 AM, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> While reading the function I decided to write some tests.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  tests/test-cutils.c | 91
>>>  +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 91 insertions(+)
>>
>> I accepted v1 because it was better than no tests at all, but did make
>> some suggestions for additional tests to perform.  I'm surprised you
>> didn't include any of those suggestions in v2.  For example, it would be
>> nice if the testsuite documents a contract on what happens with a bogus
>> suffix: is "1234x" outright rejected, or does it parse as "1234" leaving
>> the pointer at 'x'?
> 
> I thought you were ok with this patch as is.

If there was no other reason for a respin.  But once you are doing a
respin, you might as well consider it, or at least document after the
--- that you at least thought about it and had a reason for not doing it.

> But I can add some failing tests if you want (although it feels like testing strtod() at this point.

Not quite. strtod() doesn't parse suffixes, but does consume input
anyways; it then requires the user to do post-processing (so by itself
it is not an easy contract).  The whole point of writing our own wrapper
is so that we can have sane semantics without the caller having to do
post-processing.  That should include a sane contract for what we do on
a suffix we don't recognize is useful.  In particular, there could be
arguments for both "parse as much as possible", and for "refuse to parse
anything that has trailing garbage"; and since I don't know which way it
currently is, it means we SHOULD be making it part of the contract and
giving it some testing.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 4/4] Replace strotok() by strtok_r()

[Paolo Bonzini]

On 18/09/2015 12:59, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block/archipelago.c | 10 +++++-----
>  block/sheepdog.c    |  5 +++--
>  qom/cpu.c           | 12 ++++++------
>  target-i386/cpu.c   |  6 +++---
>  target-sparc/cpu.c  | 10 +++++-----
>  vl.c                |  6 +++---
>  6 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/block/archipelago.c b/block/archipelago.c
> index 855655c..1a8fcc0 100644
> --- a/block/archipelago.c
> +++ b/block/archipelago.c
> @@ -354,17 +354,17 @@ static void parse_filename_opts(const char *filename, Error **errp,
>                                  xport *mport, xport *vport)
>  {
>      const char *start;
> -    char *tokens[4], *ds;
> +    char *tokens[4], *ds, *save;
>      int idx;
>      xport lmport = NoPort, lvport = NoPort;
>  
>      strstart(filename, "archipelago:", &start);
>  
>      ds = g_strdup(start);
> -    tokens[0] = strtok(ds, "/");
> -    tokens[1] = strtok(NULL, ":");
> -    tokens[2] = strtok(NULL, ":");
> -    tokens[3] = strtok(NULL, "\0");
> +    tokens[0] = strtok_r(ds, "/", &save);
> +    tokens[1] = strtok_r(NULL, ":", &save);
> +    tokens[2] = strtok_r(NULL, ":", &save);
> +    tokens[3] = strtok_r(NULL, "\0", &save);
>  
>      if (!strlen(tokens[0])) {
>          error_setg(errp, "volume name must be specified first");
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 67ca788..272a4ca 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1614,12 +1614,13 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt)
>  {
>      struct SheepdogInode *inode = &s->inode;
>      const char *n1, *n2;
> +    char *save;
>      long copy, parity;
>      char p[10];
>  
>      pstrcpy(p, sizeof(p), opt);
> -    n1 = strtok(p, ":");
> -    n2 = strtok(NULL, ":");
> +    n1 = strtok_r(p, ":", &save);
> +    n2 = strtok_r(NULL, ":", &save);
>  
>      if (!n1) {
>          return -EINVAL;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index fb80d13a..9511f10 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -42,14 +42,14 @@ bool cpu_exists(int64_t id)
>  
>  CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
>  {
> -    char *str, *name, *featurestr;
> +    char *str, *name, *featurestr, *save;
>      CPUState *cpu;
>      ObjectClass *oc;
>      CPUClass *cc;
>      Error *err = NULL;
>  
>      str = g_strdup(cpu_model);
> -    name = strtok(str, ",");
> +    name = strtok_r(str, ",", &save);
>  
>      oc = cpu_class_by_name(typename, name);
>      if (oc == NULL) {
> @@ -60,7 +60,7 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
>      cpu = CPU(object_new(object_class_get_name(oc)));
>      cc = CPU_GET_CLASS(cpu);
>  
> -    featurestr = strtok(NULL, ",");
> +    featurestr = strtok_r(NULL, ",", &save);
>      cc->parse_features(cpu, featurestr, &err);
>      g_free(str);
>      if (err != NULL) {
> @@ -276,10 +276,10 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
>                                        Error **errp)
>  {
>      char *featurestr; /* Single "key=value" string being parsed */
> -    char *val;
> +    char *val, *save;
>      Error *err = NULL;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> +    featurestr = features ? strtok_r(features, ",", &save) : NULL;
>  
>      while (featurestr) {
>          val = strchr(featurestr, '=');
> @@ -296,7 +296,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
>                         featurestr);
>              return;
>          }
> -        featurestr = strtok(NULL, ",");
> +        featurestr = strtok_r(NULL, ",", &save);
>      }
>  }
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d2b6bc5..2487641 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1851,7 +1851,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                                       Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> -    char *featurestr; /* Single 'key=value" string being parsed */
> +    char *save, *featurestr; /* Single 'key=value" string being parsed */
>      FeatureWord w;
>      /* Features to be added */
>      FeatureWordArray plus_features = { 0 };
> @@ -1861,7 +1861,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>      CPUX86State *env = &cpu->env;
>      Error *local_err = NULL;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> +    featurestr = features ? strtok_r(features, ",", &save) : NULL;
>  
>      while (featurestr) {
>          char *val;
> @@ -1930,7 +1930,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>              error_propagate(errp, local_err);
>              return;
>          }
> -        featurestr = strtok(NULL, ",");
> +        featurestr = strtok_r(NULL, ",", &save);
>      }
>  
>      if (cpu->host_features) {
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index 9528e3a..6269091 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -95,7 +95,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      CPUSPARCState *env = &cpu->env;
>      char *s = g_strdup(cpu_model);
> -    char *featurestr, *name = strtok(s, ",");
> +    char *featurestr, *name = strtok_r(s, ",", &save);
>      sparc_def_t def1, *def = &def1;
>      Error *err = NULL;
>  
> @@ -107,7 +107,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
>      env->def = g_new0(sparc_def_t, 1);
>      memcpy(env->def, def, sizeof(*def));
>  
> -    featurestr = strtok(NULL, ",");
> +    featurestr = strtok_r(NULL, ",", &save);
>      cc->parse_features(CPU(cpu), featurestr, &err);
>      g_free(s);
>      if (err) {
> @@ -560,13 +560,13 @@ static void sparc_cpu_parse_features(CPUState *cs, char *features,
>  {
>      SPARCCPU *cpu = SPARC_CPU(cs);
>      sparc_def_t *cpu_def = cpu->env.def;
> -    char *featurestr;
> +    char *featurestr, *save;
>      uint32_t plus_features = 0;
>      uint32_t minus_features = 0;
>      uint64_t iu_version;
>      uint32_t fpu_version, mmu_version, nwindows;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> +    featurestr = features ? strtok_r(features, ",", &save) : NULL;
>      while (featurestr) {
>          char *val;
>  
> @@ -634,7 +634,7 @@ static void sparc_cpu_parse_features(CPUState *cs, char *features,
>                               "(+feature|-feature|feature=xyz)", featurestr);
>              return;
>          }
> -        featurestr = strtok(NULL, ",");
> +        featurestr = strtok_r(NULL, ",", &save);
>      }
>      cpu_def->features |= plus_features;
>      cpu_def->features &= ~minus_features;
> diff --git a/vl.c b/vl.c
> index 3c6480d..bab2696 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1323,16 +1323,16 @@ static int add_semihosting_arg(void *opaque,
>  /* Use strings passed via -kernel/-append to initialize semihosting.argv[] */
>  static inline void semihosting_arg_fallback(const char *file, const char *cmd)
>  {
> -    char *cmd_token;
> +    char *cmd_token, *save;
>  
>      /* argv[0] */
>      add_semihosting_arg(&semihosting, "arg", file, NULL);
>  
>      /* split -append and initialize argv[1..n] */
> -    cmd_token = strtok(g_strdup(cmd), " ");
> +    cmd_token = strtok_r(g_strdup(cmd), " ", &save);
>      while (cmd_token) {
>          add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
> -        cmd_token = strtok(NULL, " ");
> +        cmd_token = strtok_r(NULL, " ", &save);
>      }
>  }
>  
> 

Unfortunately mingw doesn't have strtok_r (I think).  I have queued v1
of your patches, if you want to write more tests please send them on top.

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] Replace strotok() by strtok_r()

[Marc-André Lureau]

Hi

On Fri, Sep 18, 2015 at 5:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Unfortunately mingw doesn't have strtok_r (I think).  I have queued v1
> of your patches, if you want to write more tests please send them on top.

Right, I missed that. I could take a strtok_r() from public domain for
mingw fallback though.

Thanks for taking v1 anyway.


-- 
Marc-André Lureau