diff mbox

[v9,11/37] qapi: Consolidate visitor small integer callbacks

Message ID 1453219845-30939-12-git-send-email-eblake@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake Jan. 19, 2016, 4:10 p.m. UTC
Commit 4e27e819 introduced optional visitor callbacks for all
sorts of int types, but no visitor has supplied any of the
callbacks for sizes less than 64 bits.  In other words, the
generic implementation based on using type_[u]int64() followed
by bounds-checking works just fine. In the interest of
simplicity, it's easier to make the visitor callback interface
not have to worry about the other sizes.

Adding some helper functions minimizes the boilerplate required
to correct FIXMEs added earlier with regards to questionable
reuse of errp, particularly now that we can guarantee from a
single file audit that value is unchanged if an error is set.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
v9: hoist some of visitor-impl.h changes into 9/35 and 10/35
v8: no change
v7: further factor out helper functions that eliminate the
questionable errp reuse
v6: split off from v5 23/46
original version also appeared in v6-v9 of subset D
---
 include/qapi/visitor-impl.h |   8 +--
 qapi/qapi-visit-core.c      | 158 +++++++++++++++++---------------------------
 2 files changed, 60 insertions(+), 106 deletions(-)

Comments

Markus Armbruster Jan. 20, 2016, 5:34 p.m. UTC | #1
Eric Blake <eblake@redhat.com> writes:

> Commit 4e27e819 introduced optional visitor callbacks for all
> sorts of int types, but no visitor has supplied any of the
> callbacks for sizes less than 64 bits.  In other words, the
> generic implementation based on using type_[u]int64() followed
> by bounds-checking works just fine. In the interest of
> simplicity, it's easier to make the visitor callback interface
> not have to worry about the other sizes.
>
> Adding some helper functions minimizes the boilerplate required
> to correct FIXMEs added earlier with regards to questionable
> reuse of errp, particularly now that we can guarantee from a
> single file audit that value is unchanged if an error is set.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: hoist some of visitor-impl.h changes into 9/35 and 10/35
> v8: no change
> v7: further factor out helper functions that eliminate the
> questionable errp reuse
> v6: split off from v5 23/46
> original version also appeared in v6-v9 of subset D
> ---
>  include/qapi/visitor-impl.h |   8 +--
>  qapi/qapi-visit-core.c      | 158 +++++++++++++++++---------------------------
>  2 files changed, 60 insertions(+), 106 deletions(-)

Nice diffstat.

>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 45c1d3e..e6399d1 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -1,7 +1,7 @@
>  /*
>   * Core Definitions for QAPI Visitor implementations
>   *
> - * Copyright (C) 2012 Red Hat, Inc.
> + * Copyright (C) 2012, 2015-2016 Red Hat, Inc.

git-log has authors @redhat.com in 2013 and 2014 as well.

>   *
>   * Author: Paolo Bonizni <pbonzini@redhat.com>
>   *
> @@ -56,12 +56,6 @@ struct Visitor
>      /* May be NULL; most useful for input visitors. */
>      void (*optional)(Visitor *v, bool *present, const char *name);
>
> -    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_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);
>      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-visit-core.c b/qapi/qapi-visit-core.c
> index 4a8ad43..a48fd4e 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -100,129 +100,89 @@ void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
>      v->type_int64(v, obj, name, errp);
>  }
>
> +static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
> +                             uint64_t max, const char *type, Error **errp)
> +{
> +    Error *err = NULL;
> +    uint64_t value = *obj;
> +
> +    v->type_uint64(v, &value, name, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +    } else if (value > max) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   name ? name : "null", type);

We should clean up this name ? name : "null" nonsense some day.

> +    } else {
> +        *obj = value;
> +    }
> +}
[...]
Eric Blake Jan. 20, 2016, 6:17 p.m. UTC | #2
On 01/20/2016 10:34 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Commit 4e27e819 introduced optional visitor callbacks for all
>> sorts of int types, but no visitor has supplied any of the
>> callbacks for sizes less than 64 bits.  In other words, the
>> generic implementation based on using type_[u]int64() followed
>> by bounds-checking works just fine. In the interest of
>> simplicity, it's easier to make the visitor callback interface
>> not have to worry about the other sizes.
>>
>> Adding some helper functions minimizes the boilerplate required
>> to correct FIXMEs added earlier with regards to questionable
>> reuse of errp, particularly now that we can guarantee from a
>> single file audit that value is unchanged if an error is set.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ---
>> v9: hoist some of visitor-impl.h changes into 9/35 and 10/35
>> v8: no change
>> v7: further factor out helper functions that eliminate the
>> questionable errp reuse
>> v6: split off from v5 23/46
>> original version also appeared in v6-v9 of subset D
>> ---
>>  include/qapi/visitor-impl.h |   8 +--
>>  qapi/qapi-visit-core.c      | 158 +++++++++++++++++---------------------------
>>  2 files changed, 60 insertions(+), 106 deletions(-)
> 
> Nice diffstat.
> 
>>
>> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
>> index 45c1d3e..e6399d1 100644
>> --- a/include/qapi/visitor-impl.h
>> +++ b/include/qapi/visitor-impl.h
>> @@ -1,7 +1,7 @@
>>  /*
>>   * Core Definitions for QAPI Visitor implementations
>>   *
>> - * Copyright (C) 2012 Red Hat, Inc.
>> + * Copyright (C) 2012, 2015-2016 Red Hat, Inc.
> 
> git-log has authors @redhat.com in 2013 and 2014 as well.

I didn't bother to check whether those edits were complex enough to
warrant claiming copyright additions; but I'm also amenable to
shortening to '2012-2016' on a respin regardless of the sizes of those
edits.  As it is, I was not very careful about adding 2016 in the v9
spin, so I may be inconsistent on which years are claimed where, in
comparison to where I felt I was adding new copyrightable content rather
than just minor fixing of existing content.

[While it's nice that we DON'T have to assign copyright to a central
organization to contribute to qemu, sometimes it is much nicer working
on FSF code where a single copyright holder makes discussions like this
moot.]

>> +    } else if (value > max) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                   name ? name : "null", type);
> 
> We should clean up this name ? name : "null" nonsense some day.

It all stems from whether the visitor is locally visiting { 'name':value
} vs. [ value ]; in an array visit, there is no direct name.

Maybe we could go a step higher; if we have:

{ 'name': [ value ] }

then we could require callers to parse value by passing in "[name]" or
"name[0]" rather than NULL.  Then audit the code to always pass in a
sensible name at the top level of a parse.  It would even extend to 2-D
arrays, via "[[name]]" or "name[0][0]".  But not in this series.
Markus Armbruster Jan. 21, 2016, 9:05 a.m. UTC | #3
Eric Blake <eblake@redhat.com> writes:

> On 01/20/2016 10:34 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Commit 4e27e819 introduced optional visitor callbacks for all
>>> sorts of int types, but no visitor has supplied any of the
>>> callbacks for sizes less than 64 bits.  In other words, the
>>> generic implementation based on using type_[u]int64() followed
>>> by bounds-checking works just fine. In the interest of
>>> simplicity, it's easier to make the visitor callback interface
>>> not have to worry about the other sizes.
>>>
>>> Adding some helper functions minimizes the boilerplate required
>>> to correct FIXMEs added earlier with regards to questionable
>>> reuse of errp, particularly now that we can guarantee from a
>>> single file audit that value is unchanged if an error is set.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> ---
>>> v9: hoist some of visitor-impl.h changes into 9/35 and 10/35
>>> v8: no change
>>> v7: further factor out helper functions that eliminate the
>>> questionable errp reuse
>>> v6: split off from v5 23/46
>>> original version also appeared in v6-v9 of subset D
>>> ---
>>>  include/qapi/visitor-impl.h |   8 +--
>>>  qapi/qapi-visit-core.c | 158
>>> +++++++++++++++++---------------------------
>>>  2 files changed, 60 insertions(+), 106 deletions(-)
>> 
>> Nice diffstat.
>> 
>>>
>>> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
>>> index 45c1d3e..e6399d1 100644
>>> --- a/include/qapi/visitor-impl.h
>>> +++ b/include/qapi/visitor-impl.h
>>> @@ -1,7 +1,7 @@
>>>  /*
>>>   * Core Definitions for QAPI Visitor implementations
>>>   *
>>> - * Copyright (C) 2012 Red Hat, Inc.
>>> + * Copyright (C) 2012, 2015-2016 Red Hat, Inc.
>> 
>> git-log has authors @redhat.com in 2013 and 2014 as well.
>
> I didn't bother to check whether those edits were complex enough to
> warrant claiming copyright additions; but I'm also amenable to
> shortening to '2012-2016' on a respin regardless of the sizes of those
> edits.  As it is, I was not very careful about adding 2016 in the v9
> spin, so I may be inconsistent on which years are claimed where, in
> comparison to where I felt I was adding new copyrightable content rather
> than just minor fixing of existing content.

IANAL, but I feel claiming 2012-2016 when some of the years in the
middle saw only changes of debatable copyrightability is a pardonable
sin.

> [While it's nice that we DON'T have to assign copyright to a central
> organization to contribute to qemu, sometimes it is much nicer working
> on FSF code where a single copyright holder makes discussions like this
> moot.]
>
>>> +    } else if (value > max) {
>>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>> +                   name ? name : "null", type);
>> 
>> We should clean up this name ? name : "null" nonsense some day.
>
> It all stems from whether the visitor is locally visiting { 'name':value
> } vs. [ value ]; in an array visit, there is no direct name.
>
> Maybe we could go a step higher; if we have:
>
> { 'name': [ value ] }
>
> then we could require callers to parse value by passing in "[name]" or
> "name[0]" rather than NULL.  Then audit the code to always pass in a
> sensible name at the top level of a parse.  It would even extend to 2-D
> arrays, via "[[name]]" or "name[0][0]".  But not in this series.

"Parameter 'null' expects int32_t" is not an acceptable error message.
It's better than crashing, but that's about it.

'null' is actively misleading.  The thing we choked on isn't named
'null'.  Even calling it a parameter is questionable.

A fix needs to identify the thing we choked on in a way that let's the
user find it.  '[[name]]' doesn't cut it, I'm afraid.

'expects int32_t' is below par, too.  Sure, a programmer will understand
it fine, but we need to explain things to the user in the user's term,
not in C programming jargon.
diff mbox

Patch

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 45c1d3e..e6399d1 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -1,7 +1,7 @@ 
 /*
  * Core Definitions for QAPI Visitor implementations
  *
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012, 2015-2016 Red Hat, Inc.
  *
  * Author: Paolo Bonizni <pbonzini@redhat.com>
  *
@@ -56,12 +56,6 @@  struct Visitor
     /* May be NULL; most useful for input visitors. */
     void (*optional)(Visitor *v, bool *present, const char *name);

-    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_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);
     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-visit-core.c b/qapi/qapi-visit-core.c
index 4a8ad43..a48fd4e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -100,129 +100,89 @@  void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
     v->type_int64(v, obj, name, errp);
 }

+static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
+                             uint64_t max, const char *type, Error **errp)
+{
+    Error *err = NULL;
+    uint64_t value = *obj;
+
+    v->type_uint64(v, &value, name, &err);
+    if (err) {
+        error_propagate(errp, err);
+    } else if (value > max) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", type);
+    } else {
+        *obj = value;
+    }
+}
+
 void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
 {
-    uint64_t value;
-
-    if (v->type_uint8) {
-        v->type_uint8(v, obj, name, errp);
-    } else {
-        value = *obj;
-        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");
-            return;
-        }
-        *obj = value;
-    }
+    uint64_t value = *obj;
+    visit_type_uintN(v, &value, name, UINT8_MAX, "uint8_t", errp);
+    *obj = value;
 }

-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+                       Error **errp)
 {
-    uint64_t value;
-
-    if (v->type_uint16) {
-        v->type_uint16(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_uint64(v, &value, name, errp);
-        if (value > UINT16_MAX) {
-            /* FIXME questionable reuse of errp if type_uint64() changes
-               value on error */
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "uint16_t");
-            return;
-        }
-        *obj = value;
-    }
+    uint64_t value = *obj;
+    visit_type_uintN(v, &value, name, UINT16_MAX, "uint16_t", errp);
+    *obj = value;
 }

-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp)
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+                       Error **errp)
 {
-    uint64_t value;
-
-    if (v->type_uint32) {
-        v->type_uint32(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_uint64(v, &value, name, errp);
-        if (value > UINT32_MAX) {
-            /* FIXME questionable reuse of errp if type_uint64() changes
-               value on error */
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "uint32_t");
-            return;
-        }
-        *obj = value;
-    }
+    uint64_t value = *obj;
+    visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
+    *obj = value;
 }

-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+                       Error **errp)
 {
     v->type_uint64(v, obj, name, errp);
 }

+static void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
+                            int64_t min, int64_t max, const char *type,
+                            Error **errp)
+{
+    Error *err = NULL;
+    int64_t value = *obj;
+
+    v->type_int64(v, &value, name, &err);
+    if (err) {
+        error_propagate(errp, err);
+    } else if (value < min || value > max) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   name ? name : "null", type);
+    } else {
+        *obj = value;
+    }
+}
+
 void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
-
-    if (v->type_int8) {
-        v->type_int8(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int64(v, &value, name, errp);
-        if (value < INT8_MIN || value > INT8_MAX) {
-            /* FIXME questionable reuse of errp if type_int64() changes
-               value on error */
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "int8_t");
-            return;
-        }
-        *obj = value;
-    }
+    int64_t value = *obj;
+    visit_type_intN(v, &value, name, INT8_MIN, INT8_MAX, "int8_t", errp);
+    *obj = value;
 }

 void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
-
-    if (v->type_int16) {
-        v->type_int16(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int64(v, &value, name, errp);
-        if (value < INT16_MIN || value > INT16_MAX) {
-            /* FIXME questionable reuse of errp if type_int64() changes
-               value on error */
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "int16_t");
-            return;
-        }
-        *obj = value;
-    }
+    int64_t value = *obj;
+    visit_type_intN(v, &value, name, INT16_MIN, INT16_MAX, "int16_t", errp);
+    *obj = value;
 }

 void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)
 {
-    int64_t value;
-
-    if (v->type_int32) {
-        v->type_int32(v, obj, name, errp);
-    } else {
-        value = *obj;
-        v->type_int64(v, &value, name, errp);
-        if (value < INT32_MIN || value > INT32_MAX) {
-            /* FIXME questionable reuse of errp if type_int64() changes
-               value on error */
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                       name ? name : "null", "int32_t");
-            return;
-        }
-        *obj = value;
-    }
+    int64_t value = *obj;
+    visit_type_intN(v, &value, name, INT32_MIN, INT32_MAX, "int32_t", errp);
+    *obj = value;
 }

 void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)