diff mbox

[v9,18/37] qapi: Drop unused error argument for list and implicit struct

Message ID 1453219845-30939-19-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Jan. 19, 2016, 4:10 p.m. UTC 
No backend was setting an error when ending the visit of a list
or implicit struct.  Make the callers a bit easier to follow by
making this a part of the contract, and removing the errp
argument - callers can then unconditionally end an object as
part of cleanup without having to think about whether a second
error is dominated by a first, because there is no second error.

The only addition of &error_abort in this patch, in the function
qmp_input_end_list(), will never trigger unless a programming
bug creates a push(struct)/pop(list) or push(list)/pop(struct)
mismatch.

A later patch will then tackle the larger task of splitting
visit_end_struct(), which can indeed set an error (and that
cleanup will also have the side-effect of removing the use of
error_abort added here).

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

---
v9: enhance commit message
v8: no change
v7: place earlier in series, rebase to earlier changes
v6: new patch, split from RFC on v5 7/46
---
 hw/ppc/spapr_drc.c           |  6 +-----
 include/qapi/visitor-impl.h  |  6 ++++--
 include/qapi/visitor.h       |  5 +++--
 qapi/opts-visitor.c          |  2 +-
 qapi/qapi-dealloc-visitor.c  |  4 ++--
 qapi/qapi-visit-core.c       |  8 ++++----
 qapi/qmp-input-visitor.c     |  9 ++-------
 qapi/qmp-output-visitor.c    |  2 +-
 qapi/string-input-visitor.c  |  2 +-
 qapi/string-output-visitor.c |  2 +-
 scripts/qapi-visit.py        | 10 +++-------
 11 files changed, 23 insertions(+), 33 deletions(-)

Comments

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

> No backend was setting an error when ending the visit of a list
> or implicit struct.

That's a lie: qmp_input_end_list() does.  But it shouldn't, as you
explain below.  Rephrase the commit message?

>                      Make the callers a bit easier to follow by
> making this a part of the contract, and removing the errp
> argument - callers can then unconditionally end an object as
> part of cleanup without having to think about whether a second
> error is dominated by a first, because there is no second error.
>
> The only addition of &error_abort in this patch, in the function
> qmp_input_end_list(), will never trigger unless a programming
> bug creates a push(struct)/pop(list) or push(list)/pop(struct)
> mismatch.
>
> A later patch will then tackle the larger task of splitting
> visit_end_struct(), which can indeed set an error (and that
> cleanup will also have the side-effect of removing the use of
> error_abort added here).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Patch looks good.  I like the simplification.
Eric Blake Jan. 20, 2016, 9:58 p.m. UTC | #2 
On 01/20/2016 12:03 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> No backend was setting an error when ending the visit of a list
>> or implicit struct.
> 
> That's a lie: qmp_input_end_list() does.  But it shouldn't, as you
> explain below.  Rephrase the commit message?

I'm not sure why you call it a lie - qmp_input_end_list() will not set
an error unless it is mistakenly paired with a push(struct), which none
of our code base does.  Or put another way, although qmp_input_pop()
[called by qmp_input_end_list()] has a signature that can set an error,
closer inspection shows that it will only do so when invoked to close
out a struct, and not when closing out a list.  But that's a blatant
programmer mismatch, which none of our code base does, so no well-formed
use of visitors can cause qmp_input_end_list() to set an error.

> 
>>                      Make the callers a bit easier to follow by
>> making this a part of the contract, and removing the errp
>> argument - callers can then unconditionally end an object as
>> part of cleanup without having to think about whether a second
>> error is dominated by a first, because there is no second error.
>>
>> The only addition of &error_abort in this patch, in the function
>> qmp_input_end_list(), will never trigger unless a programming
>> bug creates a push(struct)/pop(list) or push(list)/pop(struct)
>> mismatch.

I'm open to wording suggestions.

Maybe replace all of the above with:

None of the existing .end_implicit_struct() implementations use errp.
And of the existing .end_list() implementations, only
qmp_input_end_list() even uses errp, but closer inspection shows that it
will never be modified (errp is only passed to qmp_input_pop(), which
will only set an error if the corresponding push was a struct rather
than a list).  We can turn that internal usage into an &error_abort, to
protect against programmer mistakes of push(struct)/pop(list) or
push(list)/pop(struct) mismatch.

With that done, we can then make all public uses of
visit_end_implicit_struct() and visit_end_list() easier to follow by
removing the errp argument and making error-free operation part of the
contract.  Callers can then unconditionally end an object as part of
cleanup without having to think about whether a second error is
dominated by a first, because there is no possibility of a second error.

>>
>> A later patch will then tackle the larger task of splitting
>> visit_end_struct(), which can indeed set an error (and that
>> cleanup will also have the side-effect of removing the use of
>> error_abort added here).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Patch looks good.  I like the simplification.

Would help to split this into two patches, one switching from
qmp_input_pop(errp) into qmp_input_pop(&error_abort), and the other then
removing unused errp argument?
Markus Armbruster Jan. 21, 2016, 9:47 a.m. UTC | #3 
Eric Blake <eblake@redhat.com> writes:

> On 01/20/2016 12:03 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> No backend was setting an error when ending the visit of a list
>>> or implicit struct.
>> 
>> That's a lie: qmp_input_end_list() does.  But it shouldn't, as you
>> explain below.  Rephrase the commit message?
>
> I'm not sure why you call it a lie - qmp_input_end_list() will not set
> an error unless it is mistakenly paired with a push(struct), which none
> of our code base does.  Or put another way, although qmp_input_pop()
> [called by qmp_input_end_list()] has a signature that can set an error,
> closer inspection shows that it will only do so when invoked to close
> out a struct, and not when closing out a list.  But that's a blatant
> programmer mismatch, which none of our code base does, so no well-formed
> use of visitors can cause qmp_input_end_list() to set an error.

Okay, it's not a lie if you consider the whole program.  It looks like a
lie locally.

>>>                      Make the callers a bit easier to follow by
>>> making this a part of the contract, and removing the errp
>>> argument - callers can then unconditionally end an object as
>>> part of cleanup without having to think about whether a second
>>> error is dominated by a first, because there is no second error.
>>>
>>> The only addition of &error_abort in this patch, in the function
>>> qmp_input_end_list(), will never trigger unless a programming
>>> bug creates a push(struct)/pop(list) or push(list)/pop(struct)
>>> mismatch.
>
> I'm open to wording suggestions.
>
> Maybe replace all of the above with:
>
> None of the existing .end_implicit_struct() implementations use errp.
> And of the existing .end_list() implementations, only
> qmp_input_end_list() even uses errp, but closer inspection shows that it
> will never be modified (errp is only passed to qmp_input_pop(), which
> will only set an error if the corresponding push was a struct rather
> than a list).  We can turn that internal usage into an &error_abort, to
> protect against programmer mistakes of push(struct)/pop(list) or
> push(list)/pop(struct) mismatch.
>
> With that done, we can then make all public uses of
> visit_end_implicit_struct() and visit_end_list() easier to follow by
> removing the errp argument and making error-free operation part of the
> contract.  Callers can then unconditionally end an object as part of
> cleanup without having to think about whether a second error is
> dominated by a first, because there is no possibility of a second error.

Works for me.

>>> A later patch will then tackle the larger task of splitting
>>> visit_end_struct(), which can indeed set an error (and that
>>> cleanup will also have the side-effect of removing the use of
>>> error_abort added here).
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 
>> Patch looks good.  I like the simplification.
>
> Would help to split this into two patches, one switching from
> qmp_input_pop(errp) into qmp_input_pop(&error_abort), and the other then
> removing unused errp argument?

If it results in more readable commit messages.
diff mbox

Patch

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 78d71e2..ffc2cd9 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -314,11 +314,7 @@  static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
                     return;
                 }
             }
-            visit_end_list(v, &err);
-            if (err) {
-                error_propagate(errp, err);
-                return;
-            }
+            visit_end_list(v);
             break;
         }
         default:
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 6abfda7..7f512cf 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -24,11 +24,13 @@  struct Visitor

     void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
                                   Error **errp);
-    void (*end_implicit_struct)(Visitor *v, Error **errp);
+    /* May be NULL */
+    void (*end_implicit_struct)(Visitor *v);

     void (*start_list)(Visitor *v, const char *name, Error **errp);
     GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
-    void (*end_list)(Visitor *v, Error **errp);
+    /* Must be set */
+    void (*end_list)(Visitor *v);

     void (*type_enum)(Visitor *v, const char *name, int *obj,
                       const char *const strings[], Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 4abc180..10390d2 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -32,10 +32,11 @@  void visit_start_struct(Visitor *v, const char *name, void **obj,
 void visit_end_struct(Visitor *v, Error **errp);
 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp);
-void visit_end_implicit_struct(Visitor *v, Error **errp);
+void visit_end_implicit_struct(Visitor *v);
+
 void visit_start_list(Visitor *v, const char *name, Error **errp);
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
-void visit_end_list(Visitor *v, Error **errp);
+void visit_end_list(Visitor *v);

 /**
  * Check if an optional member @name of an object needs visiting.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 6d4a91e..62ffdd4 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -269,7 +269,7 @@  opts_next_list(Visitor *v, GenericList **list, Error **errp)


 static void
-opts_end_list(Visitor *v, Error **errp)
+opts_end_list(Visitor *v)
 {
     OptsVisitor *ov = to_ov(v);

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 49e7cf0..560feb3 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -83,7 +83,7 @@  static void qapi_dealloc_start_implicit_struct(Visitor *v,
     qapi_dealloc_push(qov, obj);
 }

-static void qapi_dealloc_end_implicit_struct(Visitor *v, Error **errp)
+static void qapi_dealloc_end_implicit_struct(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void **obj = qapi_dealloc_pop(qov);
@@ -119,7 +119,7 @@  static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
     return NULL;
 }

-static void qapi_dealloc_end_list(Visitor *v, Error **errp)
+static void qapi_dealloc_end_list(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void *obj = qapi_dealloc_pop(qov);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index b0452cf..2d3743b 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -37,10 +37,10 @@  void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
     }
 }

-void visit_end_implicit_struct(Visitor *v, Error **errp)
+void visit_end_implicit_struct(Visitor *v)
 {
     if (v->end_implicit_struct) {
-        v->end_implicit_struct(v, errp);
+        v->end_implicit_struct(v);
     }
 }

@@ -54,9 +54,9 @@  GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
     return v->next_list(v, list, errp);
 }

-void visit_end_list(Visitor *v, Error **errp)
+void visit_end_list(Visitor *v)
 {
-    v->end_list(v, errp);
+    v->end_list(v);
 }

 bool visit_start_union(Visitor *v, bool data_present, Error **errp)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index bf25249..597652c 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -153,10 +153,6 @@  static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
     }
 }

-static void qmp_input_end_implicit_struct(Visitor *v, Error **errp)
-{
-}
-
 static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
@@ -201,11 +197,11 @@  static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
     return entry;
 }

-static void qmp_input_end_list(Visitor *v, Error **errp)
+static void qmp_input_end_list(Visitor *v)
 {
     QmpInputVisitor *qiv = to_qiv(v);

-    qmp_input_pop(qiv, errp);
+    qmp_input_pop(qiv, &error_abort);
 }

 static void qmp_input_get_next_type(Visitor *v, const char *name, QType *type,
@@ -352,7 +348,6 @@  QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.start_struct = qmp_input_start_struct;
     v->visitor.end_struct = qmp_input_end_struct;
     v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
-    v->visitor.end_implicit_struct = qmp_input_end_implicit_struct;
     v->visitor.start_list = qmp_input_start_list;
     v->visitor.next_list = qmp_input_next_list;
     v->visitor.end_list = qmp_input_end_list;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index db5e618..d367148 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -151,7 +151,7 @@  static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
     return list ? list->next : NULL;
 }

-static void qmp_output_end_list(Visitor *v, Error **errp)
+static void qmp_output_end_list(Visitor *v)
 {
     QmpOutputVisitor *qov = to_qov(v);
     qmp_output_pop(qov);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 5347b61..610c233 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -178,7 +178,7 @@  next_list(Visitor *v, GenericList **list, Error **errp)
 }

 static void
-end_list(Visitor *v, Error **errp)
+end_list(Visitor *v)
 {
     StringInputVisitor *siv = to_siv(v);
     siv->head = true;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 74de6b6..fd917a4 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -303,7 +303,7 @@  next_list(Visitor *v, GenericList **list, Error **errp)
 }

 static void
-end_list(Visitor *v, Error **errp)
+end_list(Visitor *v)
 {
     StringOutputVisitor *sov = to_sov(v);

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8a741b6..573bb81 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -62,7 +62,7 @@  static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
     visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
         visit_type_%(c_type)s_fields(v, obj, errp);
-        visit_end_implicit_struct(v, &err);
+        visit_end_implicit_struct(v);
     }
     error_propagate(errp, err);
 }
@@ -167,9 +167,7 @@  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
         visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
     }

-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_list(v, &err);
+    visit_end_list(v);
 out:
     error_propagate(errp, err);
 }
@@ -230,9 +228,7 @@  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
                    "%(name)s");
     }
 out_obj:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_implicit_struct(v, &err);
+    visit_end_implicit_struct(v);
 out:
     error_propagate(errp, err);
 }