Re: [Qemu-devel] [PATCH v10 23/25] qapi: Drop unused error argument for list and implicit struct

[Markus Armbruster]

Eric Blake  writes:

> On 02/01/2016 06:07 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> No backend was setting an error when ending an implicit struct,
>>> or when iterating a list.
>> 
>> Perhaps "when ending the visit of a list or implicit struct, or when
>> moving to the next list node" would be more precise.  If you like it, I
>> can do that on commit.

Done.

>>>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.
>>>
>>> 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).
>
> Oh, while you're touching this up, the last half of this sentence is now
> stale (since the addition of _abort was split out into 22/25
> instead); I'd just delete the entire parenthetical, ending with "indeed
> set an error."

Done.

Re: [Qemu-devel] [PATCH v10 23/25] qapi: Drop unused error argument for list and implicit struct

[Eric Blake]

On 02/01/2016 06:07 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> No backend was setting an error when ending an implicit struct,
>> or when iterating a list.
> 
> Perhaps "when ending the visit of a list or implicit struct, or when
> moving to the next list node" would be more precise.  If you like it, I
> can do that on commit.
> 
>>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.
>>
>> 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).

Oh, while you're touching this up, the last half of this sentence is now
stale (since the addition of _abort was split out into 22/25
instead); I'd just delete the entire parenthetical, ending with "indeed
set an error."

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 23/25] qapi: Drop unused error argument for list and implicit struct

[Markus Armbruster]

Eric Blake  writes:

> No backend was setting an error when ending an implicit struct,
> or when iterating a list.

Perhaps "when ending the visit of a list or implicit struct, or when
moving to the next list node" would be more precise.  If you like it, I
can do that on commit.

>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.
>
> 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 
>
> ---
> v10: split out qmp input changes, also fix visit_next_list(), drop R-b
> 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

Patch looks good.

Re: [Qemu-devel] [PATCH v10 23/25] qapi: Drop unused error argument for list and implicit struct

[Eric Blake]

On 02/01/2016 06:07 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> No backend was setting an error when ending an implicit struct,
>> or when iterating a list.
> 
> Perhaps "when ending the visit of a list or implicit struct, or when
> moving to the next list node" would be more precise.  If you like it, I
> can do that on commit.

Yes, that sounds better.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v10 23/25] qapi: Drop unused error argument for list and implicit struct

[Eric Blake]

No backend was setting an error when ending an implicit struct,
or when iterating a list.  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.

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 

---
v10: split out qmp input changes, also fix visit_next_list(), drop R-b
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
---
 include/qapi/visitor.h   |  8 +---
 include/qapi/visitor-impl.h  |  9 ++---
 scripts/qapi-visit.py| 12 
 qapi/qapi-visit-core.c   | 12 ++--
 hw/ppc/spapr_drc.c   |  6 +-
 qapi/opts-visitor.c  |  6 +++---
 qapi/qapi-dealloc-visitor.c  |  8 
 qapi/qmp-input-visitor.c | 11 +++
 qapi/qmp-output-visitor.c|  6 +++---
 qapi/string-input-visitor.c  |  8 +++-
 qapi/string-output-visitor.c |  8 +++-
 11 files changed, 41 insertions(+), 53 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 997555d..5e581dc 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -1,6 +1,7 @@
 /*
  * Core Definitions for QAPI Visitor Classes
  *
+ * Copyright (C) 2012-2016 Red Hat, Inc.
  * Copyright IBM, Corp. 2011
  *
  * Authors:
@@ -32,10 +33,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);
+GenericList *visit_next_list(Visitor *v, GenericList **list);
+void visit_end_list(Visitor *v);

 /**
  * Check if an optional member @name of an object needs visiting.
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 337f999..ea252f8 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -24,11 +24,14 @@ 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 */
+GenericList *(*next_list)(Visitor *v, GenericList **list);
+/* 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/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 308000f..0fdcebc 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), );
 if (!err) {
 visit_type_%(c_type)s_fields(v, obj, errp);
-visit_end_implicit_struct(v, );
+visit_end_implicit_struct(v);
 }
 error_propagate(errp, err);
 }
@@ -161,15 +161,13 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 }

 for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(v, prev, )) != NULL;
+ !err && (i = visit_next_list(v, prev)) != NULL;
  prev = ) {
 %(c_name)s *native_i = (%(c_name)s *)i;
 visit_type_%(c_elt_type)s(v, NULL, _i->value, );
 }

-error_propagate(errp, err);
-err = NULL;
-visit_end_list(v, );
+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, );
+visit_end_implicit_struct(v);
 out:
 error_propagate(errp, err);
 }
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 295..89599e8 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -38,10 +38,10 @@ void visit_start_implicit_struct(Visitor *v, void **obj, 
size_t size,
 }
 }

-void