[v6,02/23] qapi: Require int64/uint64 implementation

Now that all visitors supply both type_int64() and type_uint64()
callbacks, we can drop the redundant type_int() callback (the
public interface visit_type_int() remains, but calls into
type_int64() under the hood).

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, but stems from v5 23/46
---
 include/qapi/visitor-impl.h  |  1 -
 qapi/opts-visitor.c          |  1 -
 qapi/qapi-dealloc-visitor.c  |  1 -
 qapi/qapi-visit-core.c       | 50 ++++++++++++++------------------------------
 qapi/qmp-input-visitor.c     |  1 -
 qapi/qmp-output-visitor.c    |  1 -
 qapi/string-input-visitor.c  |  1 -
 qapi/string-output-visitor.c |  1 -
 8 files changed, 16 insertions(+), 41 deletions(-)

Eric Blake <eblake@redhat.com> writes:

> Now that all visitors supply both type_int64() and type_uint64()
> callbacks, we can drop the redundant type_int() callback (the
> public interface visit_type_int() remains, but calls into
> type_int64() under the hood).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: new patch, but stems from v5 23/46
> ---
>  include/qapi/visitor-impl.h  |  1 -
>  qapi/opts-visitor.c          |  1 -
>  qapi/qapi-dealloc-visitor.c  |  1 -
>  qapi/qapi-visit-core.c       | 50 ++++++++++++++------------------------------
>  qapi/qmp-input-visitor.c     |  1 -
>  qapi/qmp-output-visitor.c    |  1 -
>  qapi/string-input-visitor.c  |  1 -
>  qapi/string-output-visitor.c |  1 -
>  8 files changed, 16 insertions(+), 41 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 44a21b7..70326e0 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -36,7 +36,6 @@ struct Visitor
>      void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
>                            const char *name, Error **errp);
>
> -    void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
>      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,
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index bc2b976..8104e42 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -522,7 +522,6 @@ opts_visitor_new(const QemuOpts *opts)
>       */
>      ov->visitor.type_enum = &input_type_enum;
>
> -    ov->visitor.type_int    = &opts_type_int64;
>      ov->visitor.type_int64  = &opts_type_int64;
>      ov->visitor.type_uint64 = &opts_type_uint64;
>      ov->visitor.type_size   = &opts_type_size;
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 5d1b2e6..5133d37 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -220,7 +220,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.next_list = qapi_dealloc_next_list;
>      v->visitor.end_list = qapi_dealloc_end_list;
>      v->visitor.type_enum = qapi_dealloc_type_enum;
> -    v->visitor.type_int = qapi_dealloc_type_int64;
>      v->visitor.type_int64 = qapi_dealloc_type_int64;
>      v->visitor.type_uint64 = qapi_dealloc_type_uint64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6d63e40..88bed9c 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -97,19 +97,19 @@ void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
>
>  void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
>  {
> -    v->type_int(v, obj, name, errp);
> +    v->type_int64(v, obj, name, 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_int(v, &value, name, errp);
> -        if (value < 0 || value > UINT8_MAX) {
> +        v->type_uint64(v, &value, name, errp);
> +        if (value > UINT8_MAX) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                         name ? name : "null", "uint8_t");
>              return;

Note that this relies on value being in range after type_uint64() fails.
If it isn't, we call error_setg() with non-null *errp.

Two solutions:

1. Stipulate that type_uint64() & friends leave value alone on error.
   Works, because its initial value *obj is in range.

2. Avoid using value on error.  A clean way to do this:

        Error *err = NULL;

        value = *obj;
        v->type_uint64(v, &value, name, &err);
        if (err) {
            error_propagate(errp, err);
            return;
        }
        if (value < 0 || value > UINT8_MAX) {
            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                       name ? name : "null", "uint8_t");
            return;
        }
        *obj = value;

   More boilerplate.  If we pick this solution, we'll want a separate
   PATCH 1.5 cleaning up the preexisting instances.

> @@ -120,14 +120,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_int(v, &value, name, errp);
> -        if (value < 0 || value > UINT16_MAX) {
> +        v->type_uint64(v, &value, name, errp);
> +        if (value > UINT16_MAX) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                         name ? name : "null", "uint16_t");
>              return;

Good.  The fewer signed-unsigned conversions, the better.

[...]

On 11/27/2015 05:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Now that all visitors supply both type_int64() and type_uint64()
>> callbacks, we can drop the redundant type_int() callback (the
>> public interface visit_type_int() remains, but calls into
>> type_int64() under the hood).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>>  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_int(v, &value, name, errp);
>> -        if (value < 0 || value > UINT8_MAX) {
>> +        v->type_uint64(v, &value, name, errp);
>> +        if (value > UINT8_MAX) {
>>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                         name ? name : "null", "uint8_t");
>>              return;
> 
> Note that this relies on value being in range after type_uint64() fails.
> If it isn't, we call error_setg() with non-null *errp.
> 
> Two solutions:
> 
> 1. Stipulate that type_uint64() & friends leave value alone on error.
>    Works, because its initial value *obj is in range.

Pre-existing and simpler, but sets a poor example for the rest of the
code base (not everyone is going to read the fine print for why it works
here), and requires cross-file audits to ensure visitors comply.

> 
> 2. Avoid using value on error.  A clean way to do this:
> 
>         Error *err = NULL;
> 
>         value = *obj;
>         v->type_uint64(v, &value, name, &err);
>         if (err) {
>             error_propagate(errp, err);
>             return;
>         }
>         if (value < 0 || value > UINT8_MAX) {
>             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                        name ? name : "null", "uint8_t");
>             return;
>         }
>         *obj = value;
> 
>    More boilerplate.  If we pick this solution, we'll want a separate
>    PATCH 1.5 cleaning up the preexisting instances.

Of course, if I do the cleanup as 1.5, then patch 3/23 reindents
everything, that's a lot of churn.  So I may end up rearranging 2 and 3
after all, and then do the cleanup as 3.5.

Or maybe option 3, write a pair of helper functions containing the
boilerplate for checking against min and max:

void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
                     int64_t min, int64_t max, Error **errp);
void visit_type_uintN(Visitor *v, int64_t *obj, const char *name,
                      uint64_t max, Error **errp);

leaving us with simpler clients:

visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
{
    int64_t value = *obj;
    visit_type_uintN(v, &value, name, INT8_MIN, INT8_MAX, errp);
    *obj = value;
}

and here, because the helpers are in the same file, it's easier to prove
that value was unchanged on error.  Or I may even squash 2 and 3 into a
single patch now.

Eric Blake <eblake@redhat.com> writes:

> On 11/27/2015 05:05 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Now that all visitors supply both type_int64() and type_uint64()
>>> callbacks, we can drop the redundant type_int() callback (the
>>> public interface visit_type_int() remains, but calls into
>>> type_int64() under the hood).
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>
>>>  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_int(v, &value, name, errp);
>>> -        if (value < 0 || value > UINT8_MAX) {
>>> +        v->type_uint64(v, &value, name, errp);
>>> +        if (value > UINT8_MAX) {
>>>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>>                         name ? name : "null", "uint8_t");
>>>              return;
>> 
>> Note that this relies on value being in range after type_uint64() fails.
>> If it isn't, we call error_setg() with non-null *errp.
>> 
>> Two solutions:
>> 
>> 1. Stipulate that type_uint64() & friends leave value alone on error.
>>    Works, because its initial value *obj is in range.
>
> Pre-existing and simpler, but sets a poor example for the rest of the
> code base (not everyone is going to read the fine print for why it works
> here), and requires cross-file audits to ensure visitors comply.

Yes, but "don't mess up externally visible state on error" is a pretty
common design maxim.

>> 2. Avoid using value on error.  A clean way to do this:
>> 
>>         Error *err = NULL;
>> 
>>         value = *obj;
>>         v->type_uint64(v, &value, name, &err);
>>         if (err) {
>>             error_propagate(errp, err);
>>             return;
>>         }
>>         if (value < 0 || value > UINT8_MAX) {
>>             error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                        name ? name : "null", "uint8_t");
>>             return;
>>         }
>>         *obj = value;
>> 
>>    More boilerplate.  If we pick this solution, we'll want a separate
>>    PATCH 1.5 cleaning up the preexisting instances.
>
> Of course, if I do the cleanup as 1.5, then patch 3/23 reindents
> everything, that's a lot of churn.  So I may end up rearranging 2 and 3
> after all, and then do the cleanup as 3.5.
>
> Or maybe option 3, write a pair of helper functions containing the
> boilerplate for checking against min and max:
>
> void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
>                      int64_t min, int64_t max, Error **errp);
> void visit_type_uintN(Visitor *v, int64_t *obj, const char *name,
>                       uint64_t max, Error **errp);
>
> leaving us with simpler clients:
>
> visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
> {
>     int64_t value = *obj;
>     visit_type_uintN(v, &value, name, INT8_MIN, INT8_MAX, errp);
>     *obj = value;
> }
>
> and here, because the helpers are in the same file, it's easier to prove
> that value was unchanged on error.  Or I may even squash 2 and 3 into a
> single patch now.

Artistic license applies.