From patchwork Fri Jan 29 13:48:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 8163401 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 71426BEEE5 for ; Fri, 29 Jan 2016 13:55:03 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 61CD1201C8 for ; Fri, 29 Jan 2016 13:55:02 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1D83B20155 for ; Fri, 29 Jan 2016 13:55:01 +0000 (UTC) Received: from localhost ([::1]:34266 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP9Vs-0005oq-HZ for patchwork-qemu-devel@patchwork.kernel.org; Fri, 29 Jan 2016 08:55:00 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP9QI-0003aN-MG for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:49:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aP9QG-0001K3-UA for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:49:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47384) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP9QG-0001Js-Lc for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:49:12 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 4669B13353; Fri, 29 Jan 2016 13:49:12 +0000 (UTC) Received: from red.redhat.com (ovpn-113-130.phx2.redhat.com [10.3.113.130]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0TDn5FO019157; Fri, 29 Jan 2016 08:49:11 -0500 From: Eric Blake To: qemu-devel@nongnu.org Date: Fri, 29 Jan 2016 06:48:50 -0700 Message-Id: <1454075341-13658-15-git-send-email-eblake@redhat.com> In-Reply-To: <1454075341-13658-1-git-send-email-eblake@redhat.com> References: <1454075341-13658-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: marcandre.lureau@redhat.com, armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [PATCH v10 14/25] qapi: Make all visitors supply uint64 callbacks X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Our qapi visitor contract supports multiple integer visitors, but left the type_uint64 visitor as optional (falling back on type_int64); which in turn can lead to awkward behavior with numbers larger than INT64_MAX (the user has to be aware of twos complement, and deal with negatives). This patch does not address the disparity in handling large values as negatives. It merely moves the fallback from uint64 to int64 from the visitor core to the visitors, where the issue can actually be fixed, by implementing the missing type_uint64() callbacks on top of the respective type_int64() callbacks, and with a FIXME comment explaining why that's wrong. With that done, we now have a type_uint64() callback in every driver, so we can make it mandatory from the core. And although the type_int64() callback can cover the entire valid range of type_uint{8,16,32} on valid user input, using type_uint64() to avoid mixed signedness makes more sense. Signed-off-by: Eric Blake --- v10: improve commit message, split out dealloc type_size change v9: hoist in part of 11/35, drop Marc-Andre's R-b v8: no change v7: split off int64 callbacks and retitle, add more FIXMEs in the code, hoist use of type_uint64 here from 3/23, improved commit message v6: new patch, but stems from v5 23/46 --- include/qapi/visitor-impl.h | 9 ++++++--- qapi/qapi-visit-core.c | 36 +++++++++++------------------------- qapi/qapi-dealloc-visitor.c | 6 ++++++ qapi/qmp-input-visitor.c | 17 +++++++++++++++++ qapi/qmp-output-visitor.c | 9 +++++++++ qapi/string-input-visitor.c | 15 +++++++++++++++ qapi/string-output-visitor.c | 9 +++++++++ 7 files changed, 73 insertions(+), 28 deletions(-) diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 319efe8..92c4bcb 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -40,6 +40,12 @@ struct Visitor void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp); /* Must be set. */ + void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, + Error **errp); + /* Optional; fallback is type_uint64(). */ + void (*type_size)(Visitor *v, uint64_t *obj, const char *name, + Error **errp); + /* Must be set. */ void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp); void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp); void (*type_number)(Visitor *v, double *obj, const char *name, @@ -53,12 +59,9 @@ struct Visitor void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp); void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp); void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp); - void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error **errp); void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp); void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp); void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp); - /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */ - void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp); bool (*start_union)(Visitor *v, bool data_present, Error **errp); }; diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 3a888ab..ac5a861 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -96,14 +96,14 @@ void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp) { - int64_t value; + uint64_t value; if (v->type_uint8) { v->type_uint8(v, obj, name, errp); } else { value = *obj; - v->type_int64(v, &value, name, errp); - if (value < 0 || value > UINT8_MAX) { + v->type_uint64(v, &value, name, errp); + if (value > UINT8_MAX) { /* FIXME questionable reuse of errp if callback changed value on error */ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, @@ -116,14 +116,14 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp) void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp) { - int64_t value; + uint64_t value; if (v->type_uint16) { v->type_uint16(v, obj, name, errp); } else { value = *obj; - v->type_int64(v, &value, name, errp); - if (value < 0 || value > UINT16_MAX) { + v->type_uint64(v, &value, name, errp); + if (value > UINT16_MAX) { /* FIXME questionable reuse of errp if callback changed value on error */ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, @@ -136,14 +136,14 @@ void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp) { - int64_t value; + uint64_t value; if (v->type_uint32) { v->type_uint32(v, obj, name, errp); } else { value = *obj; - v->type_int64(v, &value, name, errp); - if (value < 0 || value > UINT32_MAX) { + v->type_uint64(v, &value, name, errp); + if (value > UINT32_MAX) { /* FIXME questionable reuse of errp if callback changed value on error */ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, @@ -156,15 +156,7 @@ void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) { - int64_t value; - - if (v->type_uint64) { - v->type_uint64(v, obj, name, errp); - } else { - value = *obj; - v->type_int64(v, &value, name, errp); - *obj = value; - } + v->type_uint64(v, obj, name, errp); } void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp) @@ -234,16 +226,10 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp) void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp) { - int64_t value; - if (v->type_size) { v->type_size(v, obj, name, errp); - } else if (v->type_uint64) { - v->type_uint64(v, obj, name, errp); } else { - value = *obj; - v->type_int64(v, &value, name, errp); - *obj = value; + v->type_uint64(v, obj, name, errp); } } diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 8ecfc10..11eb828 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -140,6 +140,11 @@ static void qapi_dealloc_type_int64(Visitor *v, int64_t *obj, const char *name, { } +static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj, + const char *name, Error **errp) +{ +} + static void qapi_dealloc_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) { @@ -215,6 +220,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.end_list = qapi_dealloc_end_list; v->visitor.type_enum = qapi_dealloc_type_enum; v->visitor.type_int64 = qapi_dealloc_type_int64; + v->visitor.type_uint64 = qapi_dealloc_type_uint64; v->visitor.type_bool = qapi_dealloc_type_bool; v->visitor.type_str = qapi_dealloc_type_str; v->visitor.type_number = qapi_dealloc_type_number; diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 0d8a3c3..32b60bb 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -239,6 +239,22 @@ static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *name, *obj = qint_get_int(qint); } +static void qmp_input_type_uint64(Visitor *v, uint64_t *obj, const char *name, + Error **errp) +{ + /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ + QmpInputVisitor *qiv = to_qiv(v); + QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); + + if (!qint) { + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "integer"); + return; + } + + *obj = qint_get_int(qint); +} + static void qmp_input_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) { @@ -342,6 +358,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.end_list = qmp_input_end_list; v->visitor.type_enum = input_type_enum; v->visitor.type_int64 = qmp_input_type_int64; + v->visitor.type_uint64 = qmp_input_type_uint64; v->visitor.type_bool = qmp_input_type_bool; v->visitor.type_str = qmp_input_type_str; v->visitor.type_number = qmp_input_type_number; diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 3984011..f8eebaa 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -165,6 +165,14 @@ static void qmp_output_type_int64(Visitor *v, int64_t *obj, const char *name, qmp_output_add(qov, name, qint_from_int(*obj)); } +static void qmp_output_type_uint64(Visitor *v, uint64_t *obj, const char *name, + Error **errp) +{ + /* FIXME: QMP outputs values larger than INT64_MAX as negative */ + QmpOutputVisitor *qov = to_qov(v); + qmp_output_add(qov, name, qint_from_int(*obj)); +} + static void qmp_output_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) { @@ -242,6 +250,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void) v->visitor.end_list = qmp_output_end_list; v->visitor.type_enum = output_type_enum; v->visitor.type_int64 = qmp_output_type_int64; + v->visitor.type_uint64 = qmp_output_type_uint64; v->visitor.type_bool = qmp_output_type_bool; v->visitor.type_str = qmp_output_type_str; v->visitor.type_number = qmp_output_type_number; diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 2f422f0..d7546b5 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -226,6 +226,20 @@ error: "an int64 value or range"); } +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name, + Error **errp) +{ + /* FIXME: parse_type_int64 mishandles values over INT64_MAX */ + int64_t i; + Error *err = NULL; + parse_type_int64(v, &i, name, &err); + if (err) { + error_propagate(errp, err); + } else { + *obj = i; + } +} + static void parse_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp) { @@ -336,6 +350,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) v->visitor.type_enum = input_type_enum; v->visitor.type_int64 = parse_type_int64; + 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; diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index c0a9331..3ed2b2c 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -197,6 +197,14 @@ static void print_type_int64(Visitor *v, int64_t *obj, const char *name, } } +static void print_type_uint64(Visitor *v, uint64_t *obj, const char *name, + Error **errp) +{ + /* FIXME: print_type_int64 mishandles values over INT64_MAX */ + int64_t i = *obj; + print_type_int64(v, &i, name, errp); +} + static void print_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp) { @@ -346,6 +354,7 @@ StringOutputVisitor *string_output_visitor_new(bool human) v->human = human; v->visitor.type_enum = output_type_enum; v->visitor.type_int64 = print_type_int64; + v->visitor.type_uint64 = print_type_uint64; v->visitor.type_size = print_type_size; v->visitor.type_bool = print_type_bool; v->visitor.type_str = print_type_str;