Re: [Qemu-devel] [PATCH v8 31/35] qapi: Rework deallocation of partial struct

[Marc-André Lureau]

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> Commit cee2dedb noticed that if you have a partial flat union
> (such as if an input parse failed due to a missing
> discriminator), calling the dealloc visitor could result in
> trying to dereference the NULL pointer. But the fix it proposed
> requires the use of a 'data' member in the union, which may or
> may not be the same size as other branches of the union
> (consider a 32-bit platform where one of the branches is an
> int64), so it feels fairly dirty.  A better fix is to tweak all
> of the generated visit_type_implicit_FOO() functions to avoid
> dereferencing NULL in the first place, by not visiting the
> fields if the struct pointer itself is not present, at which
> point we no longer even need visit_start_union().  And no one
> was implementing visit_end_union() callbacks.
>
> While rewriting the code, use patterns that are closer to what
> is used elsewhere in the generated visitors, by using 'goto'
> to cleanup labels rather than putting followup code under 'if'
> conditions.  The change keeps the contract that any successful
> use of visit_start_implicit_struct() will be paired with a
> matching visit_end_implicit_struct(), even if intermediate
> processing is skipped.  We are safe in checking *obj alone, as
> as the contract of visit_start_implicit_struct() requires a
> non-NULL obj.
>
> As an example of the changes to generated code:
> |@@ -1331,10 +1331,16 @@ static void visit_type_implicit_Blockdev
> |     Error *err = NULL;
> |
> |     visit_start_implicit_struct(v, (void **)obj, 
> sizeof(BlockdevOptionsArchipelago), &err);
> |-    if (!err) {
> |-        visit_type_BlockdevOptionsArchipelago_fields(v, obj, errp);
> |-        visit_end_implicit_struct(v);
> |+    if (err) {
> |+        goto out;
> |+    }
> |+    if (!*obj) {
> |+        goto out_obj;
> |     }
> |+    visit_type_BlockdevOptionsArchipelago_fields(v, obj, &err);
> |+out_obj:
> |+    visit_end_implicit_struct(v);
> |+out:
> |     error_propagate(errp, err);
> | }
> ...
> |@@ -1479,9 +1539,6 @@ void visit_type_BlockdevOptions(Visitor
> |     if (err) {
> |         goto out_obj;
> |     }
> |-    if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> |-        goto out_obj;
> |-    }
> |     switch ((*obj)->driver) {
> |     case BLOCKDEV_DRIVER_ARCHIPELAGO:
> |         visit_type_implicit_BlockdevOptionsArchipelago(v, 
> &(*obj)->u.archipelago, &err);
> |@@ -1570,11 +1627,6 @@ void visit_type_BlockdevOptions(Visitor
> | out_obj:
> |     error_propagate(errp, err);
> |     err = NULL;
> |-    if (*obj) {
> |-        visit_end_union(v, !!(*obj)->u.data, &err);
> |-    }
> |-    error_propagate(errp, err);
> |-    err = NULL;
> |     visit_end_struct(v, &err);
>
> Signed-off-by: Eric Blake <address@hidden>
>

Reviewed-by: Marc-André Lureau <address@hidden>

-- 
Marc-André Lureau