[Qemu-devel] [PATCH 0/7] visitor: Fix uint64 parsing for scsi-disk wwn

[Qemu-devel] [PATCH 0/7] visitor: Fix uint64 parsing for scsi-disk wwn

[Andreas Färber]

Hello,

This series fixes uint64 handling in the string input visitor.
It was inspired by a patch from Lin Ma.

Test coverage is provided for string input visitor, QOM, scsi-disk levels.

Regards,
Andreas

Andreas Färber (7):
  string-input-visitor: Fix uint64 parsing
  test-string-input-visitor: Add int test case
  test-string-input-visitor: Add uint64 test
  tests: Add QOM property unit tests
  tests: Add scsi-disk test
  cutils: Normalize qemu_strto[u]ll() signature
  string-input-visitor: Use qemu_strto[u]ll()

 MAINTAINERS                       |   2 +
 include/qemu-common.h             |   4 +-
 qapi/string-input-visitor.c       |  68 +++++++++++++++++----
 tests/Makefile                    |   6 ++
 tests/check-qom-props.c           | 120 ++++++++++++++++++++++++++++++++++++++
 tests/scsi-disk-test.c            |  83 ++++++++++++++++++++++++++
 tests/test-cutils.c               |  76 ++++++++++++------------
 tests/test-string-input-visitor.c |  31 ++++++++++
 util/cutils.c                     |   4 +-
 9 files changed, 342 insertions(+), 52 deletions(-)
 create mode 100644 tests/check-qom-props.c
 create mode 100644 tests/scsi-disk-test.c

-- 
2.1.4

[Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Andreas Färber]

All integers would get parsed by strtoll(), not handling the case of
UINT64 properties with the most significient bit set.

Implement a .type_uint64 visitor callback, reusing the existing
parse_str() code through a new argument, using strtoull().

As a bug fix, ignore warnings about preference of qemu_strto[u]ll().

Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qapi/string-input-visitor.c | 57 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index bbd6a54..cf81f85 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -37,7 +37,7 @@ static void free_range(void *range, void *dummy)
     g_free(range);
 }
 
-static void parse_str(StringInputVisitor *siv, Error **errp)
+static void parse_str(StringInputVisitor *siv, bool u64, Error **errp)
 {
     char *str = (char *) siv->string;
     long long start, end;
@@ -50,7 +50,11 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 
     do {
         errno = 0;
-        start = strtoll(str, &endptr, 0);
+        if (u64) {
+            start = strtoull(str, &endptr, 0);
+        } else {
+            start = strtoll(str, &endptr, 0);
+        }
         if (errno == 0 && endptr > str) {
             if (*endptr == '\0') {
                 cur = g_malloc0(sizeof(*cur));
@@ -60,7 +64,7 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                                                           range_compare);
                 cur = NULL;
                 str = NULL;
-            } else if (*endptr == '-') {
+            } else if (*endptr == '-' && !u64) {
                 str = endptr + 1;
                 errno = 0;
                 end = strtoll(str, &endptr, 0);
@@ -122,7 +126,7 @@ start_list(Visitor *v, const char *name, Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
 
-    parse_str(siv, errp);
+    parse_str(siv, false, errp);
 
     siv->cur_range = g_list_first(siv->ranges);
     if (siv->cur_range) {
@@ -190,7 +194,7 @@ static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
         return;
     }
 
-    parse_str(siv, errp);
+    parse_str(siv, false, errp);
 
     if (!siv->ranges) {
         goto error;
@@ -221,6 +225,48 @@ error:
                "an int64 value or range");
 }
 
+static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                              Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+
+    if (!siv->string) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+                   "integer");
+        return;
+    }
+
+    parse_str(siv, true, errp);
+
+    if (!siv->ranges) {
+        goto error;
+    }
+
+    if (!siv->cur_range) {
+        Range *r;
+
+        siv->cur_range = g_list_first(siv->ranges);
+        if (!siv->cur_range) {
+            goto error;
+        }
+
+        r = siv->cur_range->data;
+        if (!r) {
+            goto error;
+        }
+
+        siv->cur = r->begin;
+    }
+
+    *obj = siv->cur;
+    siv->cur++;
+    return;
+
+error:
+    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
+               "a uint64 value or range");
+}
+
 static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
                             Error **errp)
 {
@@ -332,6 +378,7 @@ StringInputVisitor *string_input_visitor_new(const char *str)
 
     v->visitor.type_enum = input_type_enum;
     v->visitor.type_int = parse_type_int;
+    v->visitor.type_uint64 = parse_type_uint64;
     v->visitor.type_size = parse_type_size;
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
-- 
2.1.4

[Qemu-devel] [PATCH 2/7] test-string-input-visitor: Add int test case

[Andreas Färber]

In addition to -42 also parse the maximum int64.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 tests/test-string-input-visitor.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 8e3433e..3c5c3d9 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -62,6 +62,14 @@ static void test_visitor_in_int(TestInputVisitorData *data,
     visit_type_int(v, &res, NULL, &err);
     g_assert(!err);
     g_assert_cmpint(res, ==, value);
+    visitor_input_teardown(data, unused);
+
+    value = INT64_MAX;
+    v = visitor_input_test_init(data, g_strdup_printf("%" PRId64, value));
+
+    visit_type_int(v, &res, NULL, &err);
+    g_assert(!err);
+    g_assert_cmpint(res, ==, value);
 }
 
 static void test_visitor_in_intList(TestInputVisitorData *data,
-- 
2.1.4

[Qemu-devel] [PATCH 3/7] test-string-input-visitor: Add uint64 test

[Andreas Färber]

Test parsing of decimal and hexadecimal uint64 numbers with most
significient bit set.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 tests/test-string-input-visitor.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 3c5c3d9..558c156 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -72,6 +72,27 @@ static void test_visitor_in_int(TestInputVisitorData *data,
     g_assert_cmpint(res, ==, value);
 }
 
+static void test_visitor_in_uint64(TestInputVisitorData *data,
+                                   const void *unused)
+{
+    uint64_t res = 0, value = UINT64_MAX;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init(data, g_strdup_printf("%" PRIu64, value));
+
+    visit_type_uint64(v, &res, NULL, &err);
+    g_assert(!err);
+    g_assert_cmpint(res, ==, value);
+    visitor_input_teardown(data, unused);
+
+    v = visitor_input_test_init(data, g_strdup_printf("0x%" PRIx64, value));
+
+    visit_type_uint64(v, &res, NULL, &err);
+    g_assert(!err);
+    g_assert_cmpint(res, ==, value);
+}
+
 static void test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
@@ -271,6 +292,8 @@ int main(int argc, char **argv)
 
     input_visitor_test_add("/string-visitor/input/int",
                            &in_visitor_data, test_visitor_in_int);
+    input_visitor_test_add("/string-visitor/input/uint64",
+                           &in_visitor_data, test_visitor_in_uint64);
     input_visitor_test_add("/string-visitor/input/intList",
                            &in_visitor_data, test_visitor_in_intList);
     input_visitor_test_add("/string-visitor/input/bool",
-- 
2.1.4

[Qemu-devel] [PATCH 4/7] tests: Add QOM property unit tests

[Andreas Färber]

Add a test for parsing and setting a uint64 property.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 MAINTAINERS             |   1 +
 tests/Makefile          |   3 ++
 tests/check-qom-props.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 tests/check-qom-props.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 71c652b..a941cfd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1032,6 +1032,7 @@ F: include/qom/
 X: include/qom/cpu.h
 F: qom/
 X: qom/cpu.c
+F: tests/check-qom-props.c
 F: tests/qom-test.c
 
 QMP
diff --git a/tests/Makefile b/tests/Makefile
index 4063639..93116a2 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -72,6 +72,8 @@ check-unit-y += tests/check-qom-interface$(EXESUF)
 gcov-files-check-qom-interface-y = qom/object.c
 check-unit-y += tests/check-qom-proplist$(EXESUF)
 gcov-files-check-qom-proplist-y = qom/object.c
+check-unit-y += tests/check-qom-props$(EXESUF)
+gcov-files-check-qom-props-y = qom/object.c
 check-unit-y += tests/test-qemu-opts$(EXESUF)
 gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
@@ -303,6 +305,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o $(test-util-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
 tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y)
+tests/check-qom-props$(EXESUF): tests/check-qom-props.o $(test-qom-obj-y)
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
 tests/test-aio$(EXESUF): tests/test-aio.o $(test-block-obj-y)
 tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o $(test-util-obj-y)
diff --git a/tests/check-qom-props.c b/tests/check-qom-props.c
new file mode 100644
index 0000000..53e5cc0
--- /dev/null
+++ b/tests/check-qom-props.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2015 Red Hat, Inc.
+ * Copyright (c) 2015 SUSE Linux GmbH
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Author: Daniel P. Berrange <berrange@redhat.com>
+ *         Andreas Färber <afaerber@suse.com>
+ */
+
+#include <glib.h>
+
+#include "qapi/visitor.h"
+#include "qom/object.h"
+#include "qemu/module.h"
+
+
+#define TYPE_DUMMY "qemu-dummy"
+
+typedef struct DummyObject DummyObject;
+typedef struct DummyObjectClass DummyObjectClass;
+
+#define DUMMY_OBJECT(obj)                               \
+    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
+
+struct DummyObject {
+    Object parent_obj;
+
+    uint64_t u64val;
+};
+
+struct DummyObjectClass {
+    ObjectClass parent_class;
+};
+
+static void dummy_set_uint64(Object *obj, Visitor *v,
+                             void *opaque, const char *name,
+                             Error **errp)
+{
+    uint64_t *ptr = (uint64_t *)opaque;
+
+    visit_type_uint64(v, ptr, name, errp);
+}
+
+static void dummy_get_uint64(Object *obj, Visitor *v,
+                             void *opaque, const char *name,
+                             Error **errp)
+{
+    uint64_t value = *(uint64_t *)opaque;
+
+    visit_type_uint64(v, &value, name, errp);
+}
+
+static void dummy_init(Object *obj)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    object_property_add(obj, "u64val", "uint64",
+                              dummy_get_uint64,
+                              dummy_set_uint64,
+                              NULL, &dobj->u64val, NULL);
+}
+
+
+static const TypeInfo dummy_info = {
+    .name          = TYPE_DUMMY,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(DummyObject),
+    .instance_init = dummy_init,
+    .class_size = sizeof(DummyObjectClass),
+};
+
+static void test_dummy_uint64(void)
+{
+    Error *err = NULL;
+    char *str;
+    DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+
+    g_assert(dobj->u64val == 0);
+
+    str = g_strdup_printf("%" PRIu64, UINT64_MAX);
+    object_property_parse(OBJECT(dobj), str, "u64val", &err);
+    g_free(str);
+    g_assert(!err);
+    g_assert_cmpint(dobj->u64val, ==, UINT64_MAX);
+
+    dobj->u64val = 0;
+    str = g_strdup_printf("0x%" PRIx64, UINT64_MAX);
+    object_property_parse(OBJECT(dobj), str, "u64val", &err);
+    g_free(str);
+    g_assert(!err);
+    g_assert_cmpint(dobj->u64val, ==, UINT64_MAX);
+
+    object_unref(OBJECT(dobj));
+}
+
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    module_call_init(MODULE_INIT_QOM);
+    type_register_static(&dummy_info);
+
+    g_test_add_func("/qom/props/uint64", test_dummy_uint64);
+
+    return g_test_run();
+}
-- 
2.1.4

[Qemu-devel] [PATCH 5/7] tests: Add scsi-disk test

[Andreas Färber]

Test scsi-{disk,hd,cd} wwn properties for correct 64-bit parsing.

For now piggyback on virtio-scsi.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 MAINTAINERS            |  1 +
 tests/Makefile         |  3 ++
 tests/scsi-disk-test.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 tests/scsi-disk-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a941cfd..2078f7f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -688,6 +688,7 @@ M: Paolo Bonzini <pbonzini@redhat.com>
 S: Supported
 F: include/hw/scsi*
 F: hw/scsi/*
+F: tests/scsi-disk-test.c
 T: git git://github.com/bonzini/qemu.git scsi-next
 
 LSI53C895A
diff --git a/tests/Makefile b/tests/Makefile
index 93116a2..e9cf3cc 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -106,6 +106,8 @@ check-qtest-virtio-y += tests/virtio-rng-test$(EXESUF)
 gcov-files-virtio-y += hw/virtio/virtio-rng.c
 check-qtest-virtio-y += tests/virtio-scsi-test$(EXESUF)
 gcov-files-virtio-y += i386-softmmu/hw/scsi/virtio-scsi.c
+check-qtest-virtio-y += tests/scsi-disk-test$(EXESUF)
+gcov-files-virtio-y += i386-softmmu/hw/scsi/scsi-disk.c
 ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
 check-qtest-virtio-y += tests/virtio-9p-test$(EXESUF)
 gcov-files-virtio-y += hw/9pfs/virtio-9p.c
@@ -435,6 +437,7 @@ tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y)
+tests/scsi-disk-test$(EXESUF): tests/scsi-disk-test.o
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y)
diff --git a/tests/scsi-disk-test.c b/tests/scsi-disk-test.c
new file mode 100644
index 0000000..618fec9
--- /dev/null
+++ b/tests/scsi-disk-test.c
@@ -0,0 +1,83 @@
+/*
+ * QTest testcase for SCSI disks
+ * See virtio-scsi-test for more integrated tests.
+ *
+ * Copyright (c) 2015 SUSE Linux GmbH
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <string.h>
+#include "libqtest.h"
+#include "qemu/osdep.h"
+#include "qapi/qmp/qint.h"
+
+static void test_scsi_disk_common(const char *type, const char *id)
+{
+    char *cmdline, *path;
+    QDict *response;
+    QInt *value;
+
+    cmdline = g_strdup_printf(
+        "-drive id=drv0,if=none,file=/dev/null,format=raw "
+        "-device virtio-scsi-pci,id=scsi0 "
+        "-device %s,id=%s,bus=scsi0.0,drive=drv0"
+        ",wwn=0x%" PRIx64 ",port_wwn=0x%" PRIx64,
+        type, id, UINT64_MAX, UINT64_C(1) << 63);
+    qtest_start(cmdline);
+    g_free(cmdline);
+
+    path = g_strdup_printf("/machine/peripheral/%s", id);
+
+    response = qmp("{ 'execute': 'qom-get',"
+                   "  'arguments': { 'path': %s,"
+                   "                 'property': 'wwn' } }",
+                   path);
+    g_assert(response);
+    g_assert(qdict_haskey(response, "return"));
+    value = qobject_to_qint(qdict_get(response, "return"));
+    g_assert_cmpint(qint_get_int(value), ==, UINT64_MAX);
+
+    response = qmp("{ 'execute': 'qom-get',"
+                   "  'arguments': { 'path': %s,"
+                   "                 'property': 'port_wwn' } }",
+                   path);
+    g_assert(response);
+    g_assert(qdict_haskey(response, "return"));
+    value = qobject_to_qint(qdict_get(response, "return"));
+    g_assert_cmpint(qint_get_int(value), ==, UINT64_C(1) << 63);
+
+    g_free(path);
+    qtest_end();
+}
+
+static void test_scsi_disk(void)
+{
+    test_scsi_disk_common("scsi-disk", "disk0");
+}
+
+static void test_scsi_hd(void)
+{
+    test_scsi_disk_common("scsi-hd", "hd0");
+}
+
+static void test_scsi_cd(void)
+{
+    test_scsi_disk_common("scsi-cd", "cd0");
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/scsi-disk/props", test_scsi_disk);
+    qtest_add_func("/scsi-hd/props", test_scsi_hd);
+    qtest_add_func("/scsi-cd/props", test_scsi_cd);
+
+    ret = g_test_run();
+
+    return ret;
+}
-- 
2.1.4

[Qemu-devel] [PATCH 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Andreas Färber]

Instead of using int64_t for qemu_strtoll() and uiint64_t for
qemu_strtoull(), use long long and unsigned long long as their name
implies.

The only affected callers are our test cases.

This prepares for following checkpatch's recommendation of using it more,
by making it easier to switch from POSIX to QEMU versions.
Remaining difference is const-ness.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qemu-common.h |  4 +--
 tests/test-cutils.c   | 76 +++++++++++++++++++++++++--------------------------
 util/cutils.c         |  4 +--
 3 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 01d29dd..2575152 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -208,9 +208,9 @@ int qemu_strtol(const char *nptr, const char **endptr, int base,
 int qemu_strtoul(const char *nptr, const char **endptr, int base,
                  unsigned long *result);
 int qemu_strtoll(const char *nptr, const char **endptr, int base,
-                 int64_t *result);
+                 long long *result);
 int qemu_strtoull(const char *nptr, const char **endptr, int base,
-                  uint64_t *result);
+                  unsigned long long *result);
 
 int parse_uint(const char *s, unsigned long long *value, char **endptr,
                int base);
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 0046c61..f00150e 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -794,7 +794,7 @@ static void test_qemu_strtoll_correct(void)
     const char *str = "12345 foo";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -808,7 +808,7 @@ static void test_qemu_strtoll_null(void)
 {
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(NULL, &endptr, 0, &res);
@@ -822,7 +822,7 @@ static void test_qemu_strtoll_empty(void)
     const char *str = "";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -835,7 +835,7 @@ static void test_qemu_strtoll_whitespace(void)
     const char *str = "  \t  ";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -848,7 +848,7 @@ static void test_qemu_strtoll_invalid(void)
     const char *str = "   xxxx  \t abc";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -861,7 +861,7 @@ static void test_qemu_strtoll_trailing(void)
     const char *str = "123xxx";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -876,7 +876,7 @@ static void test_qemu_strtoll_octal(void)
     const char *str = "0123";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 8, &res);
@@ -899,7 +899,7 @@ static void test_qemu_strtoll_decimal(void)
     const char *str = "0123";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 10, &res);
@@ -923,7 +923,7 @@ static void test_qemu_strtoll_hex(void)
     const char *str = "0123";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 16, &res);
@@ -947,7 +947,7 @@ static void test_qemu_strtoll_max(void)
     const char *str = g_strdup_printf("%lld", LLONG_MAX);
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -962,7 +962,7 @@ static void test_qemu_strtoll_overflow(void)
     const char *str = "99999999999999999999999999999999999999999999";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -977,7 +977,7 @@ static void test_qemu_strtoll_underflow(void)
     const char *str = "-99999999999999999999999999999999999999999999";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err  = qemu_strtoll(str, &endptr, 0, &res);
@@ -992,7 +992,7 @@ static void test_qemu_strtoll_negative(void)
     const char *str = "  \t -321";
     char f = 'X';
     const char *endptr = &f;
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, &endptr, 0, &res);
@@ -1005,7 +1005,7 @@ static void test_qemu_strtoll_negative(void)
 static void test_qemu_strtoll_full_correct(void)
 {
     const char *str = "123";
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, NULL, 0, &res);
@@ -1016,7 +1016,7 @@ static void test_qemu_strtoll_full_correct(void)
 
 static void test_qemu_strtoll_full_null(void)
 {
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(NULL, NULL, 0, &res);
@@ -1027,7 +1027,7 @@ static void test_qemu_strtoll_full_null(void)
 static void test_qemu_strtoll_full_empty(void)
 {
     const char *str = "";
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, NULL, 0, &res);
@@ -1038,7 +1038,7 @@ static void test_qemu_strtoll_full_empty(void)
 static void test_qemu_strtoll_full_negative(void)
 {
     const char *str = " \t -321";
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, NULL, 0, &res);
@@ -1050,7 +1050,7 @@ static void test_qemu_strtoll_full_negative(void)
 static void test_qemu_strtoll_full_trailing(void)
 {
     const char *str = "123xxx";
-    int64_t res = 999;
+    long long res = 999;
     int err;
 
     err = qemu_strtoll(str, NULL, 0, &res);
@@ -1062,7 +1062,7 @@ static void test_qemu_strtoll_full_max(void)
 {
 
     const char *str = g_strdup_printf("%lld", LLONG_MAX);
-    int64_t res;
+    long long res;
     int err;
 
     err = qemu_strtoll(str, NULL, 0, &res);
@@ -1076,7 +1076,7 @@ static void test_qemu_strtoull_correct(void)
     const char *str = "12345 foo";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1090,7 +1090,7 @@ static void test_qemu_strtoull_null(void)
 {
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(NULL, &endptr, 0, &res);
@@ -1104,7 +1104,7 @@ static void test_qemu_strtoull_empty(void)
     const char *str = "";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1117,7 +1117,7 @@ static void test_qemu_strtoull_whitespace(void)
     const char *str = "  \t  ";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1130,7 +1130,7 @@ static void test_qemu_strtoull_invalid(void)
     const char *str = "   xxxx  \t abc";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1143,7 +1143,7 @@ static void test_qemu_strtoull_trailing(void)
     const char *str = "123xxx";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1158,7 +1158,7 @@ static void test_qemu_strtoull_octal(void)
     const char *str = "0123";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 8, &res);
@@ -1181,7 +1181,7 @@ static void test_qemu_strtoull_decimal(void)
     const char *str = "0123";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 10, &res);
@@ -1205,7 +1205,7 @@ static void test_qemu_strtoull_hex(void)
     const char *str = "0123";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 16, &res);
@@ -1229,7 +1229,7 @@ static void test_qemu_strtoull_max(void)
     const char *str = g_strdup_printf("%llu", ULLONG_MAX);
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1244,7 +1244,7 @@ static void test_qemu_strtoull_overflow(void)
     const char *str = "99999999999999999999999999999999999999999999";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1259,7 +1259,7 @@ static void test_qemu_strtoull_underflow(void)
     const char *str = "-99999999999999999999999999999999999999999999";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err  = qemu_strtoull(str, &endptr, 0, &res);
@@ -1274,7 +1274,7 @@ static void test_qemu_strtoull_negative(void)
     const char *str = "  \t -321";
     char f = 'X';
     const char *endptr = &f;
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, &endptr, 0, &res);
@@ -1287,7 +1287,7 @@ static void test_qemu_strtoull_negative(void)
 static void test_qemu_strtoull_full_correct(void)
 {
     const char *str = "18446744073709551614";
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, NULL, 0, &res);
@@ -1298,7 +1298,7 @@ static void test_qemu_strtoull_full_correct(void)
 
 static void test_qemu_strtoull_full_null(void)
 {
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(NULL, NULL, 0, &res);
@@ -1309,7 +1309,7 @@ static void test_qemu_strtoull_full_null(void)
 static void test_qemu_strtoull_full_empty(void)
 {
     const char *str = "";
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, NULL, 0, &res);
@@ -1320,7 +1320,7 @@ static void test_qemu_strtoull_full_empty(void)
 static void test_qemu_strtoull_full_negative(void)
 {
     const char *str = " \t -321";
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, NULL, 0, &res);
@@ -1332,7 +1332,7 @@ static void test_qemu_strtoull_full_negative(void)
 static void test_qemu_strtoull_full_trailing(void)
 {
     const char *str = "18446744073709551614xxxxxx";
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, NULL, 0, &res);
@@ -1343,7 +1343,7 @@ static void test_qemu_strtoull_full_trailing(void)
 static void test_qemu_strtoull_full_max(void)
 {
     const char *str = g_strdup_printf("%lld", ULLONG_MAX);
-    uint64_t res = 999;
+    unsigned long long res = 999;
     int err;
 
     err = qemu_strtoull(str, NULL, 0, &res);
diff --git a/util/cutils.c b/util/cutils.c
index ae35198..9e865df 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -459,7 +459,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base,
  * See qemu_strtol() documentation for more info.
  */
 int qemu_strtoll(const char *nptr, const char **endptr, int base,
-                 int64_t *result)
+                 long long *result)
 {
     char *p;
     int err = 0;
@@ -482,7 +482,7 @@ int qemu_strtoll(const char *nptr, const char **endptr, int base,
  * See qemu_strtol() documentation for more info.
  */
 int qemu_strtoull(const char *nptr, const char **endptr, int base,
-                  uint64_t *result)
+                  unsigned long long *result)
 {
     char *p;
     int err = 0;
-- 
2.1.4

[Qemu-devel] [PATCH 7/7] string-input-visitor: Use qemu_strto[u]ll()

[Andreas Färber]

Switch from strtoll() and strtoull() to qemu_strtoll() and
qemu_strtoull(), as recommended by checkpatch.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qapi/string-input-visitor.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index cf81f85..859ef96 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -39,10 +39,11 @@ static void free_range(void *range, void *dummy)
 
 static void parse_str(StringInputVisitor *siv, bool u64, Error **errp)
 {
-    char *str = (char *) siv->string;
+    const char *str = siv->string;
     long long start, end;
     Range *cur;
-    char *endptr;
+    const char *endptr;
+    int ret;
 
     if (siv->ranges) {
         return;
@@ -51,11 +52,11 @@ static void parse_str(StringInputVisitor *siv, bool u64, Error **errp)
     do {
         errno = 0;
         if (u64) {
-            start = strtoull(str, &endptr, 0);
+            ret = qemu_strtoull(str, &endptr, 0, (unsigned long long *)&start);
         } else {
-            start = strtoll(str, &endptr, 0);
+            ret = qemu_strtoll(str, &endptr, 0, &start);
         }
-        if (errno == 0 && endptr > str) {
+        if (ret == 0 && endptr > str) {
             if (*endptr == '\0') {
                 cur = g_malloc0(sizeof(*cur));
                 cur->begin = start;
@@ -67,8 +68,8 @@ static void parse_str(StringInputVisitor *siv, bool u64, Error **errp)
             } else if (*endptr == '-' && !u64) {
                 str = endptr + 1;
                 errno = 0;
-                end = strtoll(str, &endptr, 0);
-                if (errno == 0 && endptr > str && start <= end &&
+                ret = qemu_strtoll(str, &endptr, 0, &end);
+                if (ret == 0 && endptr > str && start <= end &&
                     (start > INT64_MAX - 65536 ||
                      end < start + 65536)) {
                     if (*endptr == '\0') {
-- 
2.1.4

Re: [Qemu-devel] [PATCH 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Paolo Bonzini]

On 25/09/2015 14:39, Andreas Färber wrote:
> Instead of using int64_t for qemu_strtoll() and uiint64_t for
> qemu_strtoull(), use long long and unsigned long long as their name
> implies.
> 
> The only affected callers are our test cases.
> 
> This prepares for following checkpatch's recommendation of using it more,
> by making it easier to switch from POSIX to QEMU versions.
> Remaining difference is const-ness.

Do we actually use long long and unsigned long long anywhere?

Paolo

Re: [Qemu-devel] [PATCH 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Andreas Färber]

Am 25.09.2015 um 14:42 schrieb Paolo Bonzini:
> On 25/09/2015 14:39, Andreas Färber wrote:
>> Instead of using int64_t for qemu_strtoll() and uiint64_t for
>> qemu_strtoull(), use long long and unsigned long long as their name
>> implies.
>>
>> The only affected callers are our test cases.
>>
>> This prepares for following checkpatch's recommendation of using it more,
>> by making it easier to switch from POSIX to QEMU versions.
>> Remaining difference is const-ness.
> 
> Do we actually use long long and unsigned long long anywhere?

The next patch does, because checkpatch asks for it. :)

qemu_strtoull() has some special handling for Windows apparently.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Paolo Bonzini]

On 25/09/2015 14:44, Andreas Färber wrote:
> > Do we actually use long long and unsigned long long anywhere?
> 
> The next patch does, because checkpatch asks for it. :)
> 
> qemu_strtoull() has some special handling for Windows apparently.

No, I really mean the types. :) The qemu_strtoll/qemu_strtoull functions
use {,u}int64_t because they are much more used than long long and
unsigned long long.

Paolo

Re: [Qemu-devel] [PATCH 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Andreas Färber]

Am 25.09.2015 um 14:56 schrieb Paolo Bonzini:
> On 25/09/2015 14:44, Andreas Färber wrote:
>>> Do we actually use long long and unsigned long long anywhere?
>>
>> The next patch does, because checkpatch asks for it. :)
>>
>> qemu_strtoull() has some special handling for Windows apparently.
> 
> No, I really mean the types. :) The qemu_strtoll/qemu_strtoull functions
> use {,u}int64_t because they are much more used than long long and
> unsigned long long.

Well, my answer still stands: The next patch has code using long long.

Problem is that uint64_t foo = strtoull(...) works, while
qemu_strtoull(..., &foo) causes a pointer mismatch warning treated as
error. I could've converted those to uint64_t (assuming the type is not
needed for something else), but I rather wanted to keep changes small.

If we want functions using [u]int64_t, we should name them
...strto[u]64, not mixing C and POSIX types.

But I assumed there may be some controversy, so I intentionally put this
after the actual bug fixes and test cases, they can easily be dropped. :)

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Paolo Bonzini]

On 25/09/2015 15:27, Andreas Färber wrote:
>> > 
>> > No, I really mean the types. :) The qemu_strtoll/qemu_strtoull functions
>> > use {,u}int64_t because they are much more used than long long and
>> > unsigned long long.
> Well, my answer still stands: The next patch has code using long long.
> 
> Problem is that uint64_t foo = strtoull(...) works, while
> qemu_strtoull(..., &foo) causes a pointer mismatch warning treated as
> error. I could've converted those to uint64_t (assuming the type is not
> needed for something else), but I rather wanted to keep changes small.
> 
> If we want functions using [u]int64_t, we should name them
> ...strto[u]64, not mixing C and POSIX types.

Good point.  Yes, I would prefer the function to be renamed and using
uint64_t in string-input-visitor.

Paolo

> But I assumed there may be some controversy, so I intentionally put this
> after the actual bug fixes and test cases, they can easily be dropped. :)

Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Eric Blake]

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

On 09/25/2015 06:39 AM, Andreas Färber wrote:
> All integers would get parsed by strtoll(), not handling the case of
> UINT64 properties with the most significient bit set.
> 
> Implement a .type_uint64 visitor callback, reusing the existing
> parse_str() code through a new argument, using strtoull().
> 
> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  qapi/string-input-visitor.c | 57 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 

> @@ -50,7 +50,11 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>  
>      do {
>          errno = 0;
> -        start = strtoll(str, &endptr, 0);
> +        if (u64) {
> +            start = strtoull(str, &endptr, 0);

accepts the range [-ULLONG_MAX, ULLONG_MAX] (with 2s complement
wraparound). Do you really want -1 being a synonym for ULLONG_MAX, or do
you want to explicitly reject leading '-' when parsing unsigned
(arguments can be made for both behaviors; in fact, libvirt has two
separate wrappers for parsing uint64_t depending on which behavior is
wanted)

> +        } else {
> +            start = strtoll(str, &endptr, 0);

accepts the range [LLONG_MIN, LLONG_MAX] (that is, roughly half the
range of the unsigned version)

> +        }
>          if (errno == 0 && endptr > str) {
>              if (*endptr == '\0') {
>                  cur = g_malloc0(sizeof(*cur));
> @@ -60,7 +64,7 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>                                                            range_compare);
>                  cur = NULL;
>                  str = NULL;
> -            } else if (*endptr == '-') {
> +            } else if (*endptr == '-' && !u64) {

Why do you not want to handle ranges when using unsigned numbers?


>  
> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                              Error **errp)
> +{
> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> +
> +    if (!siv->string) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "integer");
> +        return;
> +    }
...

That's a lot of copy-and-paste. Can't you make parse_type_int64() and
parse_type_uint64() both call into a single helper method, that contains
the guts of the existing parse_type_int64() and adds a single parameter
for the one place where the two functions differ on their call to
parse_str()?

-- 
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 2/7] test-string-input-visitor: Add int test case

[Eric Blake]

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

On 09/25/2015 06:39 AM, Andreas Färber wrote:
> In addition to -42 also parse the maximum int64.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  tests/test-string-input-visitor.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Shouldn't we also add tests of one-beyond-range, to have coverage of
expected error behavior when a number is too large in magnitude?

-- 
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 3/7] test-string-input-visitor: Add uint64 test

[Eric Blake]

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

On 09/25/2015 06:39 AM, Andreas Färber wrote:
> Test parsing of decimal and hexadecimal uint64 numbers with most
> significient bit set.

s/significient/significant/

> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  tests/test-string-input-visitor.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)

It would also be nice to add tests of one-beyond-range to make sure we
are properly diagnosing range issues.  And we need to make up our mind
on whether "-1" is a parse error or accepted as a synonym for ULLONG_MAX
when doing an unsigned parse.

-- 
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 4/7] tests: Add QOM property unit tests

[Eric Blake]

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

On 09/25/2015 06:39 AM, Andreas Färber wrote:
> Add a test for parsing and setting a uint64 property.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  MAINTAINERS             |   1 +
>  tests/Makefile          |   3 ++
>  tests/check-qom-props.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 tests/check-qom-props.c
> 

> +static void test_dummy_uint64(void)
> +{
> +    Error *err = NULL;
> +    char *str;
> +    DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> +
> +    g_assert(dobj->u64val == 0);
> +
> +    str = g_strdup_printf("%" PRIu64, UINT64_MAX);
> +    object_property_parse(OBJECT(dobj), str, "u64val", &err);
> +    g_free(str);
> +    g_assert(!err);

Use &error_abort, then you don't need the g_assert(!err).

> +    g_assert_cmpint(dobj->u64val, ==, UINT64_MAX);
> +
> +    dobj->u64val = 0;
> +    str = g_strdup_printf("0x%" PRIx64, UINT64_MAX);
> +    object_property_parse(OBJECT(dobj), str, "u64val", &err);
> +    g_free(str);
> +    g_assert(!err);
> +    g_assert_cmpint(dobj->u64val, ==, UINT64_MAX);
> +
> +    object_unref(OBJECT(dobj));

As with other patches in this series, intentionally testing the behavior
of -1, and of (ULLONG_MAX+1), would be good.

-- 
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 6/7] cutils: Normalize qemu_strto[u]ll() signature

[Eric Blake]

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

On 09/25/2015 06:39 AM, Andreas Färber wrote:
> Instead of using int64_t for qemu_strtoll() and uiint64_t for

s/uiint64/uint64/

> qemu_strtoull(), use long long and unsigned long long as their name
> implies.
> 
> The only affected callers are our test cases.
> 
> This prepares for following checkpatch's recommendation of using it more,
> by making it easier to switch from POSIX to QEMU versions.
> Remaining difference is const-ness.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qemu-common.h |  4 +--
>  tests/test-cutils.c   | 76 +++++++++++++++++++++++++--------------------------
>  util/cutils.c         |  4 +--
>  3 files changed, 42 insertions(+), 42 deletions(-)

I concur with Paolo's suggestion to instead rename the functions to make
it obvious that we are parsing [u]int64, since that is more likely to
match the type that the bulk of callers will already have on hand.

-- 
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 4/7] tests: Add QOM property unit tests

[Daniel P. Berrange]

On Fri, Sep 25, 2015 at 02:39:45PM +0200, Andreas Färber wrote:
> Add a test for parsing and setting a uint64 property.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  MAINTAINERS             |   1 +
>  tests/Makefile          |   3 ++
>  tests/check-qom-props.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 tests/check-qom-props.c

FWIW, back in commit a31bdae5a76ecc060c1eb8a66be1896072c1e8b2 I added
a  check-qom-proplist.c unit test, and later extended it with test
for enums. Perhaps your addition is best placed in there, or we could
rename that test to check-qom-props.c so we just have one file for all
property unit testing

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> All integers would get parsed by strtoll(), not handling the case of
> UINT64 properties with the most significient bit set.

This mess is part of a bigger mess, I'm afraid.

The major ways integers get parsed are:

* QMP: parse_literal() in qmp/qobject/json-parser.c

  This is what parses QMP off the wire.

  RFC 7159 does not prescribe range or precision of JSON numbers.  Our
  implementation accepts the union of int64_t and double.

  If the lexer recognizes a floating-point number, we convert it with
  strtod() and represent it as double.

  If the lexer recognizes a decimal integer, and strtoll() can convert
  it, we represent it in int64_t.  Else, we convert it with strtod() and
  represent it as double.  Unclean: code assumes int64_t is long long.

  Yes, that means QMP can't currently support the full range of QAPI's
  uint64 type.

* QemuOpts: parse_option_number() in util/qemu-option.c

  This is what parses key=value,... strings for command line and other
  places.

  QemuOpts can be used in two ways.  If you fill out QemuOptDesc desc[],
  it rejects unknown keys and parses values of known keys.  If you leave
  it empty, it accepts all keys, and doesn't parse values.  Either way,
  it also stores raw string values.

  QemuOpts' parser only supports unsigned numbers, in decimal, octal and
  hex.  Error checking is very poor.  In particular, it considers
  negative input valid, and silently casts it to uint64_t.  I wouldn't
  be surprised if some code depended on that.

* String input visitor: parse_str() in qapi/string-input-visitor.c

  This appears to be used only by QOM so far:

  - object_property_get_enum()
  - object_property_get_uint16List()
  - object_property_parse()

  parse_str() appears to parse some fancy list syntax.  Comes from
  commit 659268f.  The commit message is useless.  I can't see offhand
  how this interacts with the visitor core.

  Anyway, if we ignore the fancy crap and just look at the parsing of a
  single integer, we see that it supports int64_t in decimal, octal and
  hex, it fails to check for ERANGE, and assumes int64_t is long long.

* Options visitor: opts_type_int() in opts qapi/opts-visitor.c

  This one is for converting QemuOpts to QAPI-defined C types.  It uses
  the raw string values, not the parsed ones.  The QemuOpts parser is
  neither needed nor wanted here.  You should use the options visitor
  with an empty desc[] array to bypass it.  Example: numa.c.

  We got fancy list syntax again.  This one looks like I could
  understand it with a bit of effort.  But let's look just at the
  parsing of a single integer.  It supports uint64_t in decimal, octal
  and hex, and *surprise* checks for errors carefully.

Fixing just a part of a mess can be okay.  I just don't want to lever
the bigger mess unmentioned.

> Implement a .type_uint64 visitor callback, reusing the existing
> parse_str() code through a new argument, using strtoull().

I'm afraid you're leaving the bug in the visitor core unfixed.

The (essentially undocumented) Visitor abstraction has the following
methods for integers:

* Mandatory: type_int()

  Interface uses int64_t for the value.  The implementation should
  ensure it fits into int64_t.

* Optional: type_int{8,16,32}()

  These use int{8,16,32}_t for the value.

  If present, it should ensure the value fits into the data type.

  If missing, the core falls back to type_int() plus appropriate range
  checking.

* Optional: type_int64()

  Same interface as type_int().

  If present, it should ensure the value fits into int64_t.

  If missing, the core falls back to type_int().

  Aside: setting type_int64() would be useful only when you want to
  distinguish QAPI types int and int64.  So far, nobody does.  In fact,
  nobody uses QAPI type int64!  I'm tempted to define QAPI type int as a
  mere alias for int64 and drop the redundant stuff.

* Optional: type_uint{8,16,32}()

  These use uint{8,16,32}_t for the value.

  If present, it should ensure the value fits into the data type.

  If missing, the core falls back to type_int() plus appropriate range
  checking.

* Optional: type_uint64()

  Now it gets interesting.  Interface uses uint64_t for the value.

  If present, it should ensure the value fits into uint64_t.

  If missing, the core falls back to type_int().  No range checking.  If
  type_int() performs range checking as it should, then uint64_t values
  not representable in int64_t get rejected (wrong), and negative values
  representable in int64_t get cast to uint64_t (also wrong).

  I think we need to make type_uint64() mandatory, and drop the
  fallback.

* Optional: type_size()

  Same interface as type_uint64().

  If present, it should ensure the value fits into uint64_t.

  If missing, the core first tries falling back to type_uint64() and
  then to type_int().  Falling back to type_int() is as wrong here as it
  is in type_uint64().

> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().

I'm not sure I get this sentence.

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Andreas Färber <afaerber@suse.de>

On the actual patch, I have nothing to add over Eric's review right now.

Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Andreas Färber]

Am 30.09.2015 um 15:19 schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
>> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
> 
> I'm not sure I get this sentence.

This patch causes checkpatch warnings. I intentionally do not address
them in this bug-fix patch, but instead in a later patch in the series.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Eric Blake]

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

On 09/30/2015 07:19 AM, Markus Armbruster wrote:

> 
> The (essentially undocumented) Visitor abstraction has the following
> methods for integers:

I proposed documentation at:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05434.html

> 
> * Mandatory: type_int()
> 
>   Interface uses int64_t for the value.  The implementation should
>   ensure it fits into int64_t.
> 
> * Optional: type_int{8,16,32}()
> 
>   These use int{8,16,32}_t for the value.
> 
>   If present, it should ensure the value fits into the data type.
> 
>   If missing, the core falls back to type_int() plus appropriate range
>   checking.

No one implements them.  In fact, as part of preparing my documentation,
I actually proposed simplifying the visitor callback interface to drop them:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05432.html

> 
> * Optional: type_int64()
> 
>   Same interface as type_int().
> 
>   If present, it should ensure the value fits into int64_t.
> 
>   If missing, the core falls back to type_int().
> 
>   Aside: setting type_int64() would be useful only when you want to
>   distinguish QAPI types int and int64.  So far, nobody does.  In fact,
>   nobody uses QAPI type int64!  I'm tempted to define QAPI type int as a
>   mere alias for int64 and drop the redundant stuff.

Already part of my proposal.

> 
> * Optional: type_uint{8,16,32}()
> 
>   These use uint{8,16,32}_t for the value.
> 
>   If present, it should ensure the value fits into the data type.
> 
>   If missing, the core falls back to type_int() plus appropriate range
>   checking.

Also unused, and simplified above.

> 
> * Optional: type_uint64()
> 
>   Now it gets interesting.  Interface uses uint64_t for the value.
> 
>   If present, it should ensure the value fits into uint64_t.
> 
>   If missing, the core falls back to type_int().  No range checking.  If
>   type_int() performs range checking as it should, then uint64_t values
>   not representable in int64_t get rejected (wrong), and negative values
>   representable in int64_t get cast to uint64_t (also wrong).
> 
>   I think we need to make type_uint64() mandatory, and drop the
>   fallback.

Probably a good idea, although not done in my proposed patches.

> 
> * Optional: type_size()
> 
>   Same interface as type_uint64().
> 
>   If present, it should ensure the value fits into uint64_t.
> 
>   If missing, the core first tries falling back to type_uint64() and
>   then to type_int().  Falling back to type_int() is as wrong here as it
>   is in type_uint64().

Provided by the QemuOpts parser to allow '1k' to mean 1024, and so on.

> 
>> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
> 
> I'm not sure I get this sentence.
> 
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> On the actual patch, I have nothing to add over Eric's review right now.
> 

-- 
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 1/7] string-input-visitor: Fix uint64 parsing

[Eric Blake]

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

On 09/30/2015 07:23 AM, Andreas Färber wrote:
> Am 30.09.2015 um 15:19 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>>> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
>>
>> I'm not sure I get this sentence.
> 
> This patch causes checkpatch warnings. I intentionally do not address
> them in this bug-fix patch, but instead in a later patch in the series.

Maybe:

As this is a bug fix, the patch intentionally ignores checkpatch
warnings to prefer the use of qemu_strto[u]ll() to minimize size; a
later patch will further address that issue.

-- 
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 1/7] string-input-visitor: Fix uint64 parsing

[Andreas Färber]

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

Am 25.09.2015 um 16:49 schrieb Eric Blake:
> On 09/25/2015 06:39 AM, Andreas Färber wrote:
>> All integers would get parsed by strtoll(), not handling the case of
>> UINT64 properties with the most significient bit set.
>>
>> Implement a .type_uint64 visitor callback, reusing the existing
>> parse_str() code through a new argument, using strtoull().
>>
>> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  qapi/string-input-visitor.c | 57 +++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 5 deletions(-)
>>
> 
>> @@ -50,7 +50,11 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>>  
>>      do {
>>          errno = 0;
>> -        start = strtoll(str, &endptr, 0);
>> +        if (u64) {
>> +            start = strtoull(str, &endptr, 0);
> 
> accepts the range [-ULLONG_MAX, ULLONG_MAX] (with 2s complement
> wraparound). Do you really want -1 being a synonym for ULLONG_MAX, or do
> you want to explicitly reject leading '-' when parsing unsigned
> (arguments can be made for both behaviors; in fact, libvirt has two
> separate wrappers for parsing uint64_t depending on which behavior is
> wanted)
> 
>> +        } else {
>> +            start = strtoll(str, &endptr, 0);
> 
> accepts the range [LLONG_MIN, LLONG_MAX] (that is, roughly half the
> range of the unsigned version)

No one has further commented on this, so I take it no further changes
are required here for now.

>> +        }
>>          if (errno == 0 && endptr > str) {
>>              if (*endptr == '\0') {
>>                  cur = g_malloc0(sizeof(*cur));
>> @@ -60,7 +64,7 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>>                                                            range_compare);
>>                  cur = NULL;
>>                  str = NULL;
>> -            } else if (*endptr == '-') {
>> +            } else if (*endptr == '-' && !u64) {
> 
> Why do you not want to handle ranges when using unsigned numbers?

For some reason I must've read this as handling negative numbers, which
we wouldn't have for unsigned numbers...

However, since there is only one .start_list() callback, which passes
!u64 to retain previous behavior, we would never actually run into this
code path today. I've reverted my change and duplicated the strtoull()
handling instead nonetheless.

>>  
>> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
>> +                              Error **errp)
>> +{
>> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
>> +
>> +    if (!siv->string) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> +                   "integer");
>> +        return;
>> +    }
> ...
> 
> That's a lot of copy-and-paste. Can't you make parse_type_int64() and
> parse_type_uint64() both call into a single helper method, that contains
> the guts of the existing parse_type_int64() and adds a single parameter
> for the one place where the two functions differ on their call to
> parse_str()?

I don't see how. They have different signatures, and there's a lot of
gotos that differ in the error message. I'm all for sharing code but it
seems more work refactoring that code for reuse than duplication saved.
If you have a concrete suggestion how to improve it, please share a diff
or let's do that as follow-up.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)


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

Re: [Qemu-devel] [PATCH 4/7] tests: Add QOM property unit tests

[Andreas Färber]

Am 25.09.2015 um 17:01 schrieb Daniel P. Berrange:
> On Fri, Sep 25, 2015 at 02:39:45PM +0200, Andreas Färber wrote:
>> Add a test for parsing and setting a uint64 property.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  MAINTAINERS             |   1 +
>>  tests/Makefile          |   3 ++
>>  tests/check-qom-props.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 124 insertions(+)
>>  create mode 100644 tests/check-qom-props.c
> 
> FWIW, back in commit a31bdae5a76ecc060c1eb8a66be1896072c1e8b2 I added
> a  check-qom-proplist.c unit test, and later extended it with test
> for enums.

That's where your copyright and authorship came from.

> Perhaps your addition is best placed in there, or we could
> rename that test to check-qom-props.c so we just have one file for all
> property unit testing

I can only guess that my dislike was "/qom/proplist" as my test has
nothing to do with lists. Otherwise it looks similar enough. I'll rename
it and merge the two.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)