Eric Blake <ebl...@redhat.com> writes:
> Our qapi visitor contract supports multiple integer visitors,
> but left the type_uint64 visitor as optional (falling back on
> type_int64); it also marks the type_size visitor as optional
> (falling back on type_uint64 or even type_int64).
>
> Note that the default of falling back on type_int for unsigned
> visitors can cause confusing results for values larger than
> INT64_MAX (such as having to pass in a negative 2s complement
> value on input, and getting a negative result on output).
>
> This patch does not fully address the disparity in handling
> large values as negatives, but does move towards a cleaner
> situation where EVERY visitor provides both type_int64 and
> type_uint64 variants as entry points; then each client can
> either implement sane differences between the two, or document
> in place with a FIXME that there is munging going on.
Before: nobody implements type_uint64(), and the core falls back to
type_int64(), casting negative values to large positive ones. With an
implementation of type_int64() that parses large positive values as
negative, the two casts cancel out.
After: everybody implements type_uint64() incorrectly, by reusing
type_int64() code. The problem moves from visitor core to visitor
implementations, where we can actually fix it if we care.
Correct?
> The dealloc visitor no longer needs a type_size callback,
> since that now safely falls back to the type_uint64 callback.
Did it need the callback before this patch?
> Then, in qapi-visit-core.c, we can now use the guaranteed
> type_uint64 callback as the fallback for all smaller unsigned
> int visits.
The type_int64() callback works just fine for smaller unsigned ints, but
I agree avoiding mixed signedness by using type_uint64() make sense once
type_uint64() is mandatory.
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> 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-dealloc-visitor.c | 12 ++++++------
> qapi/qapi-visit-core.c | 42 ++++++++++++++----------------------------
> 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, 76 insertions(+), 37 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index c263a26..45c1d3e 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);
> void (*end_union)(Visitor *v, bool data_present, Error **errp);
> };
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index e9b9f3f..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)
> {
> @@ -158,11 +163,6 @@ static void qapi_dealloc_type_anything(Visitor *v,
> QObject **obj,
> }
> }
>
> -static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char
> *name,
> - Error **errp)
> -{
> -}
> -
> static void qapi_dealloc_type_enum(Visitor *v, int *obj,
> const char * const strings[],
> const char *kind, const char *name,
> @@ -220,11 +220,11 @@ 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;
> v->visitor.type_any = qapi_dealloc_type_anything;
> - v->visitor.type_size = qapi_dealloc_type_size;
> v->visitor.start_union = qapi_dealloc_start_union;
>
> QTAILQ_INIT(&v->stack);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6295fa8..4a8ad43 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -102,15 +102,15 @@ 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) {
> - /* FIXME questionable reuse of errp if type_int64() changes
> + v->type_uint64(v, &value, name, errp);
> + if (value > UINT8_MAX) {
> + /* FIXME questionable reuse of errp if type_uint64() changes
> value on error */
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> name ? name : "null", "uint8_t");
You could delay adding these FIXMEs until this patch, and reduce churn.
Probably not worth the bother now.
[...]
