1. 06 Jul, 2016 2 commits
    • Eric Blake's avatar
      qapi: Add new visit_free() function · 2c0ef9f4
      Eric Blake authored
      Making each visitor provide its own (awkwardly-named) FOO_cleanup()
      is unusual, when we can instead have a polymorphic visit_free()
      interface.  Over the next few patches, we can use the polymorphic
      functions to eliminate the need for a FOO_get_visitor() function
      for accessing specific visitor functionality, once everything can
      be accessed directly through the Visitor* interfaces.
      
      The dealloc visitor is the first one converted to completely use
      the new entry point, since qapi_dealloc_visitor_cleanup() was the
      only reason that qapi_dealloc_get_visitor() existed, and only
      generated and testsuite code was even using it.  With the new
      visit_free() entry point in place, we no longer need to expose
      the QapiDeallocVisitor subtype through qapi_dealloc_visitor_new(),
      and can get by with less generated code, with diffs that look like:
      
      | void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)
      | {
      |-    QapiDeallocVisitor *qdv;
      |     Visitor *v;
      |
      |     if (!obj) {
      |         return;
      |     }
      |
      |-    qdv = qapi_dealloc_visitor_new();
      |-    v = qapi_dealloc_get_visitor(qdv);
      |+    v = qapi_dealloc_visitor_new();
      |     visit_type_ACPIOSTInfo(v, NULL, &obj, NULL);
      |-    qapi_dealloc_visitor_cleanup(qdv);
      |+    visit_free(v);
      |}
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1465490926-28625-5-git-send-email-eblake@redhat.com>
      Reviewed-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      2c0ef9f4
    • Eric Blake's avatar
      qapi: Add parameter to visit_end_* · 1158bb2a
      Eric Blake authored
      Rather than making the dealloc visitor track of stack of pointers
      remembered during visit_start_* in order to free them during
      visit_end_*, it's a lot easier to just make all callers pass the
      same pointer to visit_end_*.  The generated code has access to the
      same pointer, while all other users are doing virtual walks and
      can pass NULL.  The dealloc visitor is then greatly simplified.
      
      All three visit_end_*() functions intentionally take a void**,
      even though the visit_start_*() functions differ between void**,
      GenericList**, and GenericAlternate**.  This is done for several
      reasons: when doing a virtual walk, passing NULL doesn't care
      what the type is, but when doing a generated walk, we already
      have to cast the caller's specific FOO* to call visit_start,
      while using void** lets us use visit_end without a cast. Also,
      an upcoming patch will add a clone visitor that wants to use
      the same implementation for all three visit_end callbacks,
      which is made easier if all three share the same signature.
      
      For visitors with already track per-object state (the QMP visitors
      via a stack, and the string visitors which do not allow nesting),
      add an assertion that the caller is indeed passing the same
      pointer to paired calls.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1465490926-28625-4-git-send-email-eblake@redhat.com>
      Reviewed-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      1158bb2a
  2. 12 May, 2016 4 commits
    • Eric Blake's avatar
      qapi: Simplify semantics of visit_next_list() · d9f62dde
      Eric Blake authored
      The semantics of the list visit are somewhat baroque, with the
      following pseudocode when FooList is used:
      
      start()
      for (prev = head; cur = next(prev); prev = &cur) {
          visit(&cur->value)
      }
      
      Note that these semantics (advance before visit) requires that
      the first call to next() return the list head, while all other
      calls return the next element of the list; that is, every visitor
      implementation is required to track extra state to decide whether
      to return the input as-is, or to advance.  It also requires an
      argument of 'GenericList **' to next(), solely because the first
      iteration might need to modify the caller's GenericList head, so
      that all other calls have to do a layer of dereferencing.
      
      Thankfully, we only have two uses of list visits in the entire
      code base: one in spapr_drc (which completely avoids
      visit_next_list(), feeding in integers from a different source
      than uint8List), and one in qapi-visit.py.  That is, all other
      list visitors are generated in qapi-visit.c, and share the same
      paradigm based on a qapi FooList type, so we can refactor how
      lists are laid out with minimal churn among clients.
      
      We can greatly simplify things by hoisting the special case
      into the start() routine, and flipping the order in the loop
      to visit before advance:
      
      start(head)
      for (tail = *head; tail; tail = next(tail)) {
          visit(&tail->value)
      }
      
      With the simpler semantics, visitors have less state to track,
      the argument to next() is reduced to 'GenericList *', and it
      also becomes obvious whether an input visitor is allocating a
      FooList during visit_start_list() (rather than the old way of
      not knowing if an allocation happened until the first
      visit_next_list()).  As a minor drawback, we now allocate in
      two functions instead of one, and have to pass the size to
      both functions (unless we were to tweak the input visitors to
      cache the size to start_list for reuse during next_list, but
      that defeats the goal of less visitor state).
      
      The signature of visit_start_list() is chosen to match
      visit_start_struct(), with the new parameters after 'name'.
      
      The spapr_drc case is a virtual visit, done by passing NULL for
      list, similarly to how NULL is passed to visit_start_struct()
      when a qapi type is not used in those visits.  It was easy to
      provide these semantics for qmp-output and dealloc visitors,
      and a bit harder for qmp-input (several prerequisite patches
      refactored things to make this patch straightforward).  But it
      turned out that the string and opts visitors munge enough other
      state during visit_next_list() to make it easier to just
      document and require a GenericList visit for now; an assertion
      will remind us to adjust things if we need the semantics in the
      future.
      
      Several pre-requisite cleanup patches made the reshuffling of
      the various visitors easier; particularly the qmp input visitor.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      d9f62dde
    • Eric Blake's avatar
      qapi: Split visit_end_struct() into pieces · 15c2f669
      Eric Blake authored
      As mentioned in previous patches, we want to call visit_end_struct()
      functions unconditionally, so that visitors can release resources
      tied up since the matching visit_start_struct() without also having
      to worry about error priority if more than one error occurs.
      
      Even though error_propagate() can be safely used to ignore a second
      error during cleanup caused by a first error, it is simpler if the
      cleanup cannot set an error.  So, split out the error checking
      portion (basically, input visitors checking for unvisited keys) into
      a new function visit_check_struct(), which can be safely skipped if
      any earlier errors are encountered, and leave the cleanup portion
      (which never fails, but must be called unconditionally if
      visit_start_struct() succeeded) in visit_end_struct().
      
      Generated code in qapi-visit.c has diffs resembling:
      
      |@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v,
      |         goto out_obj;
      |     }
      |     visit_type_ACPIOSTInfo_members(v, obj, &err);
      |-    error_propagate(errp, err);
      |-    err = NULL;
      |+    if (err) {
      |+        goto out_obj;
      |+    }
      |+    visit_check_struct(v, &err);
      | out_obj:
      |-    visit_end_struct(v, &err);
      |+    visit_end_struct(v);
      | out:
      
      and in qapi-event.c:
      
      @@ -47,7 +47,10 @@ void qapi_event_send_acpi_device_ost(ACP
      |         goto out;
      |     }
      |     visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, &param, &err);
      |-    visit_end_struct(v, err ? NULL : &err);
      |+    if (!err) {
      |+        visit_check_struct(v, &err);
      |+    }
      |+    visit_end_struct(v);
      |     if (err) {
      |         goto out;
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com>
      [Conflict with a doc fixup resolved]
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      15c2f669
    • Eric Blake's avatar
      qapi: Add visit_type_null() visitor · 3bc97fd5
      Eric Blake authored
      Right now, qmp-output-visitor happens to produce a QNull result
      if nothing is actually visited between the creation of the visitor
      and the request for the resulting QObject.  A stronger protocol
      would require that a QMP output visit MUST visit something.  But
      to still be able to produce a JSON 'null' output, we need a new
      visitor function that states our intentions.  Yes, we could say
      that such a visit must go through visit_type_any(), but that
      feels clunky.
      
      So this patch introduces the new visit_type_null() interface and
      its no-op interface in the dealloc visitor, and stubs in the
      qmp visitors (the next patch will finish the implementation).
      For the visitors that will not implement the callback, document
      the situation. The code in qapi-visit-core unconditionally
      dereferences the callback pointer, so that a segfault will inform
      a developer if they need to implement the callback for their
      choice of visitor.
      
      Note that JSON has a primitive null type, with the single value
      null; likewise with the QNull type for QObject; but for QAPI,
      we just have the 'null' value without a null type.  We may
      eventually want to add more support in QAPI for null (most likely,
      we'd use it via an alternate type that permits 'null' or an
      object); but we'll create that usage when we need it.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1461879932-9020-15-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      3bc97fd5
    • Eric Blake's avatar
      qapi-visit: Add visitor.type classification · 983f52d4
      Eric Blake authored
      We have three classes of QAPI visitors: input, output, and dealloc.
      Currently, all implementations of these visitors have one thing in
      common based on their visitor type: the implementation used for the
      visit_type_enum() callback.  But since we plan to add more such
      common behavior, in relation to documenting and further refining
      the semantics, it makes more sense to have the visitor
      implementations advertise which class they belong to, so the common
      qapi-visit-core code can use that information in multiple places.
      
      A later patch will better document the types of visitors directly
      in visitor.h.
      
      For this patch, knowing the class of a visitor implementation lets
      us make input_type_enum() and output_type_enum() become static
      functions, by replacing the callback function Visitor.type_enum()
      with the simpler enum member Visitor.type.  Share a common
      assertion in qapi-visit-core as part of the refactoring.
      
      Move comments in opts-visitor.c to match the refactored layout.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1461879932-9020-2-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      983f52d4
  3. 19 Feb, 2016 3 commits
    • Eric Blake's avatar
      qapi: Change visit_start_implicit_struct to visit_start_alternate · dbf11922
      Eric Blake authored
      After recent changes, the only remaining use of
      visit_start_implicit_struct() is for allocating the space needed
      when visiting an alternate.  Since the term 'implicit struct' is
      hard to explain, rename the function to its current usage.  While
      at it, we can merge the functionality of visit_get_next_type()
      into the same function, making it more like visit_start_struct().
      
      Generated code is now slightly smaller:
      
      | {
      |     Error *err = NULL;
      |
      |-    visit_start_implicit_struct(v, (void**) obj, sizeof(BlockdevRef), &err);
      |+    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
      |+                          true, &err);
      |     if (err) {
      |         goto out;
      |     }
      |-    visit_get_next_type(v, name, &(*obj)->type, true, &err);
      |-    if (err) {
      |-        goto out_obj;
      |-    }
      |     switch ((*obj)->type) {
      |     case QTYPE_QDICT:
      |         visit_start_struct(v, name, NULL, 0, &err);
      ...
      |     }
      |-out_obj:
      |-    visit_end_implicit_struct(v);
      |+    visit_end_alternate(v);
      | out:
      |     error_propagate(errp, err);
      | }
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1455778109-6278-16-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      dbf11922
    • Eric Blake's avatar
      qapi: Don't box branches of flat unions · 544a3731
      Eric Blake authored
      There's no reason to do two malloc's for a flat union; let's just
      inline the branch struct directly into the C union branch of the
      flat union.
      
      Surprisingly, fewer clients were actually using explicit references
      to the branch types in comparison to the number of flat unions
      thus modified.
      
      This lets us reduce the hack in qapi-types:gen_variants() added in
      the previous patch; we no longer need to distinguish between
      alternates and flat unions.
      
      The change to unboxed structs means that u.data (added in commit
      cee2dedb) is now coincident with random fields of each branch of
      the flat union, whereas beforehand it was only coincident with
      pointers (since all branches of a flat union have to be objects).
      Note that this was already the case for simple unions - but there
      we got lucky.  Remember, visit_start_union() blindly returns true
      for all visitors except for the dealloc visitor, where it returns
      the value !!obj->u.data, and that this result then controls
      whether to proceed with the visit to the variant.  Pre-patch,
      this meant that flat unions were testing whether the boxed pointer
      was still NULL, and thereby skipping visit_end_implicit_struct()
      and avoiding a NULL dereference if the pointer had not been
      allocated.  The same was true for simple unions where the current
      branch had pointer type, except there we bypassed visit_type_FOO().
      But for simple unions where the current branch had scalar type, the
      contents of that scalar meant that the decision to call
      visit_type_FOO() was data-dependent - the reason we got lucky there
      is that visit_type_FOO() for all scalar types in the dealloc visitor
      is a no-op (only the pointer variants had anything to free), so it
      did not matter whether the dealloc visit was skipped.  But with this
      patch, we would risk leaking memory if we could skip a call to
      visit_type_FOO_fields() based solely on a data-dependent decision.
      
      But notice: in the dealloc visitor, visit_type_FOO() already handles
      a NULL obj - it was only the visit_type_implicit_FOO() that was
      failing to check for NULL. And now that we have refactored things to
      have the branch be part of the parent struct, we no longer have a
      separate pointer that can be NULL in the first place.  So we can just
      delete the call to visit_start_union() altogether, and blindly visit
      the branch type; there is no change in behavior except to the dealloc
      visitor, where we now unconditionally visit the branch, but where that
      visit is now always safe (for a flat union, we can no longer
      dereference NULL, and for a simple union, visit_type_FOO() was already
      safely handling NULL on pointer types).
      
      Unfortunately, simple unions are not as easy to switch to unboxed
      layout; because we are special-casing the hidden implicit type with
      a single 'data' member, we really DO need to keep calling another
      layer of visit_start_struct(), with a second malloc; although there
      are some cleanups planned for simple unions in later patches.
      
      visit_start_union() and gen_visit_implicit_struct() are now unused.
      Drop them.
      
      Note that after this patch, the only remaining use of
      visit_start_implicit_struct() is for alternate types; the next patch
      will do further cleanup based on that fact.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1455778109-6278-14-git-send-email-eblake@redhat.com>
      [Dead code deletion squashed in, commit message updated accordingly]
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      544a3731
    • Eric Blake's avatar
      qapi: Adjust layout of FooList types · e65d89bf
      Eric Blake authored
      By sticking the next pointer first, we don't need a union with
      64-bit padding for smaller types.  On 32-bit platforms, this
      can reduce the size of uint8List from 16 bytes (or 12, depending
      on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
      It has no effect on 64-bit platforms (where alignment still
      dictates a 16-byte struct); but fewer anonymous unions is still
      a win in my book.
      
      It requires visit_next_list() to gain a size parameter, to know
      what size element to allocate; comparable to the size parameter
      of visit_start_struct().
      
      I debated about going one step further, to allow for fewer casts,
      by doing:
          typedef GenericList GenericList;
          struct GenericList {
              GenericList *next;
          };
          struct FooList {
              GenericList base;
              Foo *value;
          };
      so that you convert to 'GenericList *' by '&foolist->base', and
      back by 'container_of(generic, GenericList, base)' (as opposed to
      the existing '(GenericList *)foolist' and '(FooList *)generic').
      But doing that would require hoisting the declaration of
      GenericList prior to inclusion of qapi-types.h, rather than its
      current spot in visitor.h; it also makes iteration a bit more
      verbose through 'foolist->base.next' instead of 'foolist->next'.
      
      Note that for lists of objects, the 'value' payload is still
      hidden behind a boxed pointer.  Someday, it would be nice to do:
      
      struct FooList {
          FooList *next;
          Foo value;
      };
      
      for one less level of malloc for each list element.  This patch
      is a step in that direction (now that 'next' is no longer at a
      fixed non-zero offset within the struct, we can store more than
      just a pointer's-worth of data as the value payload), but the
      actual conversion would be a task for another series, as it will
      touch a lot of code.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1455778109-6278-10-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      e65d89bf
  4. 08 Feb, 2016 7 commits
    • Eric Blake's avatar
      qapi: Drop unused error argument for list and implicit struct · 08f9541d
      Eric Blake authored
      No backend was setting an error when ending the visit of a list or
      implicit struct, or when moving to the next list node.  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.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1454075341-13658-24-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      08f9541d
    • Eric Blake's avatar
      qapi: Drop unused 'kind' for struct/enum visit · 337283df
      Eric Blake authored
      visit_start_struct() and visit_type_enum() had a 'kind' argument
      that was usually set to either the stringized version of the
      corresponding qapi type name, or to NULL (although some clients
      didn't even get that right).  But nothing ever used the argument.
      It's even hard to argue that it would be useful in a debugger,
      as a stack backtrace also tells which type is being visited.
      
      Therefore, drop the 'kind' argument as dead.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Reviewed-by: 's avatarMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-Id: <1454075341-13658-22-git-send-email-eblake@redhat.com>
      [Harmless rebase mistake cleaned up]
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      337283df
    • Eric Blake's avatar
      qapi: Swap 'name' in visit_* callbacks to match public API · 0b2a0d6b
      Eric Blake authored
      As explained in the previous patches, matching argument order of
      'name, &value' to JSON's "name":value makes sense.  However,
      while the last two patches were easy with Coccinelle, I ended up
      doing this one all by hand.  Now all the visitor callbacks match
      the main interface.
      
      The compiler is able to enforce that all clients match the changed
      interface in visitor-impl.h, even where two pointers are being
      swapped, because only one of the two pointers is const (if that
      were not the case, then C's looseness on treating 'char *' like
      'void *' would have made review a bit harder).
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Reviewed-by: 's avatarMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-Id: <1454075341-13658-21-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      0b2a0d6b
    • Eric Blake's avatar
      qapi: Make all visitors supply uint64 callbacks · f755dea7
      Eric Blake authored
      Our qapi visitor contract supports multiple integer visitors,
      but left the type_uint64 visitor as optional (falling back on
      type_int64); which in turn can lead to awkward behavior with
      numbers larger than INT64_MAX (the user has to be aware of
      twos complement, and deal with negatives).
      
      This patch does not address the disparity in handling large
      values as negatives.  It merely moves the fallback from uint64
      to int64 from the visitor core to the visitors, where the issue
      can actually be fixed, by implementing the missing type_uint64()
      callbacks on top of the respective type_int64() callbacks, and
      with a FIXME comment explaining why that's wrong.
      
      With that done, we now have a type_uint64() callback in every
      driver, so we can make it mandatory from the core.  And although
      the type_int64() callback can cover the entire valid range of
      type_uint{8,16,32} on valid user input, using type_uint64() to
      avoid mixed signedness makes more sense.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1454075341-13658-15-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      f755dea7
    • Eric Blake's avatar
      qapi: Prefer type_int64 over type_int in visitors · 4c40314a
      Eric Blake authored
      The qapi builtin type 'int' is basically shorthand for the type
      'int64'.  In fact, since no visitor was providing the optional
      type_int64() callback, visit_type_int64() was just always falling
      back to type_int(), cementing the equivalence between the types.
      
      However, some visitors are providing a type_uint64() callback.
      For purposes of code consistency, it is nicer if all visitors
      use the paired type_int64/type_uint64 names rather than the
      mismatched type_int/type_uint64.  So this patch just renames
      the signed int callbacks in place, dropping the type_int()
      callback as redundant, and a later patch will focus on the
      unsigned int callbacks.
      
      Add some FIXMEs to questionable reuse of errp in code touched
      by the rename, while at it (the reuse works as long as the
      callbacks don't modify value when setting an error, but it's not
      a good example to set) - a later patch will then fix those.
      
      No change in functionality here, although further cleanups are
      in the pipeline.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1454075341-13658-14-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      4c40314a
    • Eric Blake's avatar
      qapi: Dealloc visitor does not need a type_size() · 4894b00b
      Eric Blake authored
      The intent of having the visitor type_size() callback differ
      from type_uint64() is to allow special handling for sizes; the
      visitor core gracefully falls back to type_uint64() if there is
      no need for the distinction.  Since the dealloc visitor does
      nothing for any of the int visits, drop the pointless size
      handler.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Message-Id: <1454075341-13658-5-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      4894b00b
    • Eric Blake's avatar
      qapi: Drop dead dealloc visitor variable · 77577cb8
      Eric Blake authored
      Commit 0b9d8542 added StackEntry.is_list_head, but forgot to
      delete the now-unused QapiDeallocVisitor.is_list_head.
      Signed-off-by: 's avatarEric Blake <eblake@redhat.com>
      Reviewed-by: 's avatarMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-Id: <1454075341-13658-4-git-send-email-eblake@redhat.com>
      Signed-off-by: 's avatarMarkus Armbruster <armbru@redhat.com>
      77577cb8
  5. 04 Feb, 2016 1 commit
  6. 21 Sep, 2015 1 commit
  7. 19 Jun, 2015 1 commit
  8. 26 Sep, 2014 1 commit
  9. 09 May, 2014 1 commit
  10. 03 Mar, 2014 1 commit
  11. 06 Nov, 2013 1 commit
  12. 19 Dec, 2012 3 commits
  13. 29 Nov, 2012 1 commit
  14. 26 Nov, 2012 1 commit
  15. 04 Oct, 2011 2 commits
  16. 21 Aug, 2011 1 commit
  17. 21 Jul, 2011 1 commit