1. 19 7月, 2016 4 次提交
    • E
      qapi: Implement boxed types for commands/events · c818408e
      Eric Blake 提交于
      Turn on the ability to pass command and event arguments in
      a single boxed parameter, which must name a non-empty type
      (although the type can be a struct with all optional members).
      For structs, it makes it possible to pass a single qapi type
      instead of a breakout of all struct members (useful if the
      arguments are already in a struct or if the number of members
      is large); for other complex types, it is now possible to use
      a union or alternate as the data for a command or event.
      
      The empty type may be technically feasible if needed down the
      road, but it's easier to forbid it now and relax things to allow
      it later, than it is to allow it now and have to special case
      how the generated 'q_empty' type is handled (see commit 7ce106a9
      for reasons why nothing is generated for the empty type).  An
      alternate type is never considered empty, but now that a boxed
      type can be either an object or an alternate, we have to provide
      a trivial QAPISchemaAlternateType.is_empty().  The new call to
      arg_type.is_empty() during QAPISchemaCommand.check() requires
      that we first check the type in question; but there is no chance
      of introducing a cycle since objects do not refer back to commands.
      
      We still have a split in syntax checking between ad-hoc parsing
      up front (merely validates that 'boxed' has a sane value) and
      during .check() methods (if 'boxed' is set, then 'data' must name
      a non-empty user-defined type).
      
      Generated code is unchanged, as long as no client uses the
      new feature.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1468468228-27827-10-git-send-email-eblake@redhat.com>
      Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
      [Test files renamed to *-boxed-*]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      c818408e
    • E
      qapi: Plumb in 'boxed' to qapi generator lower levels · 48825ca4
      Eric Blake 提交于
      The next patch will add support for passing a qapi union type
      as the 'data' of a command.  But to do that, the user function
      for implementing the command, as called by the generated
      marshal command, must take the corresponding C struct as a
      single boxed pointer, rather than a breakdown into one
      parameter per member.  Even without a union, being able to use
      a C struct rather than a list of parameters can make it much
      easier to handle coding with QAPI.
      
      This patch adds the internal plumbing of a 'boxed' flag
      associated with each command and event.  In several cases,
      this means adding indentation, with one new dead branch and
      the remaining branch being the original code more deeply
      nested; this was done so that the new implementation in the
      next patch is easier to review without also being mixed with
      indentation changes.
      
      For this patch, no behavior or generated output changes, other
      than the testsuite outputting the value of the new flag
      (always False for now).
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1468468228-27827-9-git-send-email-eblake@redhat.com>
      Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
      [Identifier box renamed to boxed in two places]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      48825ca4
    • E
      qapi: Hide tag_name data member of variants · da9cb193
      Eric Blake 提交于
      Clean up the only remaining external use of the tag_name field of
      QAPISchemaObjectTypeVariants, by explicitly listing the generated
      'type' tag for all variants in the testsuite (you can still tell
      simple unions by the -wrapper types).  Then we can mark the
      tag_name field as private by adding a leading underscore to prevent
      any further use.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1468468228-27827-5-git-send-email-eblake@redhat.com>
      Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      da9cb193
    • E
      qapi: Require all branches of flat union enum to be covered · d0b18239
      Eric Blake 提交于
      We were previously enforcing that all flat union branches were
      found in the corresponding enum, but not that all enum values
      were covered by branches.  The resulting generated code would
      abort() if the user passes the uncovered enum value.
      
      We don't automatically treat non-present branches in a flat
      union as empty types, for symmetry with simple unions (there,
      the enum type is generated from the list of all branches, so
      there is no way to omit a branch but still have it be part of
      the union).
      
      A later patch will add shorthand so that branches that are empty
      in flat unions can be declared as 'branch':{} instead of
      'branch':'Empty', to avoid the need for an otherwise useless
      explicit empty type.  [Such shorthand for simple unions is a bit
      harder to justify, since we would still have to generate a
      wrapper type that parses 'data':{}, rather than truly being an
      empty branch with no additional siblings to the 'type' member.]
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1468468228-27827-3-git-send-email-eblake@redhat.com>
      Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      d0b18239
  2. 18 3月, 2016 3 次提交
    • E
      qapi: Allow anonymous base for flat union · ac4338f8
      Eric Blake 提交于
      Rather than requiring all flat unions to explicitly create
      a separate base struct, we can allow the qapi schema to specify
      the common members via an inline dictionary. This is similar to
      how commands can specify an inline anonymous type for its 'data'.
      We already have several struct types that only exist to serve as
      a single flat union's base; the next commit will clean them up.
      In particular, this patch's change to the BlockdevOptions example
      in qapi-code-gen.txt will actually be done in the real QAPI schema.
      
      Now that anonymous bases are legal, we need to rework the
      flat-union-bad-base negative test (as previously written, it
      forms what is now valid QAPI; tweak it to now provide coverage
      of a new error message path), and add a positive test in
      qapi-schema-test to use an anonymous base (making the integer
      argument optional, for even more coverage).
      
      Note that this patch only allows anonymous bases for flat unions;
      simple unions are already enough syntactic sugar that we do not
      want to burden them further.  Meanwhile, while it would be easy
      to also allow an anonymous base for structs, that would be quite
      redundant, as the members can be put right into the struct
      instead.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1458254921-17042-15-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      ac4338f8
    • E
      qapi: Adjust names of implicit types · 7599697c
      Eric Blake 提交于
      The original choice of ':obj-' as the prefix for implicit types
      made it obvious that we weren't going to clash with any user-defined
      names, which cannot contain ':'.  But now we want to create structs
      for implicit types, to get rid of special cases in the generators,
      and our use of ':' in implicit names needs a tweak to produce valid
      C code.
      
      We could transliterate ':' to '_', except that C99 mandates that
      "identifiers that begin with an underscore are always reserved for
      use as identifiers with file scope in both the ordinary and tag name
      spaces".  So it's time to change our naming convention: we can
      instead use the 'q_' prefix that we reserved for ourselves back in
      commit 9fb081e0.  Technically, since we aren't planning on exposing
      the empty type in generated code, we could keep the name ':empty',
      but renaming it to 'q_empty' makes the check for startswith('q_')
      cover all implicit types, whether or not code is generated for them.
      
      As long as we don't declare 'empty' or 'obj' ticklish, it shouldn't
      clash with c_name() prepending 'q_' to the user's ticklish names.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1458254921-17042-5-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      7599697c
    • E
      qapi: Fix command with named empty argument type · 972a1101
      Eric Blake 提交于
      The generator special-cased
      
       { 'command':'foo', 'data': {} }
      
      to avoid emitting a visitor variable, but failed to see that
      
       { 'struct':'NamedEmptyType, 'data': {} }
       { 'command':'foo', 'data':'NamedEmptyType' }
      
      needs the same treatment.  There, the generator happily generates a
      visitor to get no arguments, and a visitor to destroy no arguments;
      and the compiler isn't happy with that, as demonstrated by the updated
      qapi-schema-test.json:
      
        tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
        tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable]
             Visitor *v;
                      ^
      
      No change to generated code except for the testsuite addition.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1458254921-17042-3-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      972a1101
  3. 05 3月, 2016 2 次提交
  4. 19 2月, 2016 4 次提交
    • E
      qapi: Add tests of complex objects within alternate · 68d07839
      Eric Blake 提交于
      Upcoming patches will adjust how we visit an object branch of an
      alternate; but we were completely lacking testsuite coverage.
      Rectify this, so that the future patches will be able to highlight
      the changes and still prove that we avoided regressions.
      
      In particular, the use of a flat union UserDefFlatUnion rather
      than a simple struct UserDefA as the branch will give us coverage
      of an object with variants.  And visiting an alternate as both
      the top level and as a nested member gives confidence in correct
      memory allocation handling, especially if the test is run under
      valgrind.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1455778109-6278-5-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      68d07839
    • E
      qapi: Forbid 'any' inside an alternate · 46534309
      Eric Blake 提交于
      The whole point of an alternate is to allow some type-safety while
      still accepting more than one JSON type.  Meanwhile, the 'any'
      type exists to bypass type-safety altogether.  The two are
      incompatible: you can't accept every type, and still tell which
      branch of the alternate to use for the parse; fix this to give a
      sane error instead of a Python stack trace.
      
      Note that other types that can't be alternate members are caught
      earlier, by check_type().
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1455778109-6278-4-git-send-email-eblake@redhat.com>
      [Commit message tweaked]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      46534309
    • E
      qapi: Forbid empty unions and useless alternates · 02a57ae3
      Eric Blake 提交于
      Empty unions serve no purpose, and while we compile with gcc
      which permits them, strict C99 forbids them.  We happen to inject
      a dummy 'void *data' member into the C unions that represent QAPI
      unions and alternates, but we want to get rid of that member (it
      pollutes the namespace for no good reason), which would leave us
      with an empty union if the user didn't provide any branches.  While
      empty structs make sense in QAPI, empty unions don't add any
      expressiveness to the QMP language.  So prohibit them at parse
      time.  Update the documentation and testsuite to match.
      
      Note that the documentation already mentioned that alternates
      should have "two or more JSON data types"; so this also fixes
      the code to enforce that.  However, we have existing uses of a
      union type with only one branch, so the 2-or-more strictness
      is intentionally limited to alternates.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1455778109-6278-3-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      02a57ae3
    • E
      qapi-visit: Honor prefix of discriminator enum · 9d3524b3
      Eric Blake 提交于
      When we added support for a user-specified prefix for an enum
      type (commit 351d36e4), we forgot to teach the qapi-visit code
      to honor that prefix in the case of using a prefixed enum as
      the discriminator for a flat union.  While there is still some
      on-list debate on whether we want to keep prefixes, we should
      at least make it work as long as it is still part of the code
      base.
      Reported-by: NDaniel P. Berrange <berrange@redhat.com>
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1455665965-27638-1-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      9d3524b3
  5. 17 12月, 2015 11 次提交
    • E
      qapi: Detect base class loops · bac5429c
      Eric Blake 提交于
      It should be fairly obvious that qapi base classes need to
      form an acyclic graph, since QMP cannot specify the same
      key more than once, while base classes are included as flat
      members alongside other members added by the child.  But the
      old check_member_clash() parser function was not prepared to
      check for this, and entered an infinite recursion (at least
      until Python gives up, complaining about nesting too deep).
      
      Now that check_member_clash() has been recently removed,
      attempts at self-inheritance trigger an assertion failure
      introduced by commit ac88219a.  The obvious fix is to turn
      the assertion into a conditional.
      
      This patch includes both the tests (base-cycle-direct and
      base-cycle-indirect) and the fix, since the .err file output
      for the unfixed case is not useful (particularly when it was
      warning about unbounded recursion, as that limit may be
      platform-specific).
      
      We don't need to worry about cycles in flat unions (neither
      the base type nor the type of a variant can be a union) nor
      in alternates (alternate branches cannot themselves be an
      alternate).  But if we later allow a union type as a variant,
      we will still be okay, as QAPISchemaObjectTypeVariants.check()
      triggers the same QAPISchemaObjectType.check() that will
      detect any loops.
      
      Likewise, we need not worry about the case of diamond
      inheritance where the same class is used for a flat union base
      class and one of its variants; either both uses will introduce
      a collision in trying to insert the same member name twice, or
      the shared type is empty and changes nothing.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1449033659-25497-16-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      bac5429c
    • E
      qapi: Move duplicate collision checks to schema check() · 01cfbaa4
      Eric Blake 提交于
      With the recent commit 'qapi: Detect collisions in C member
      names', we have two different locations for detecting clashes -
      one at parse time, and another at QAPISchema*.check() time.
      Remove all of the ad hoc parser checks, and delete associated
      code (for example, the global check_member_clash() method is
      no longer needed).
      
      Testing this showed that the test union-bad-branch wasn't adding
      much: union-clash-branches also exposes the error message when
      branches collide, and we've recently fixed things to avoid an
      implicit collision with max.  Likewise, the error for
      enum-clash-member changes to report our new detection of
      upper case in a value name, unless we modify the test to use
      all lower case.
      
      The wording of several error messages has changed, but the
      change is generally an improvement rather than a regression.
      
      No change to generated code.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1449033659-25497-15-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      01cfbaa4
    • E
      qapi: Enforce (or whitelist) case conventions on qapi members · 893e1f2c
      Eric Blake 提交于
      We document that members of enums and objects should be
      'lower-case', although we were not enforcing it.  We have to
      whitelist a few pre-existing entities that violate the norms.
      Add three new tests to expose the new error message, each of
      which first uses the whitelisted name 'UuidInfo' to prove the
      whitelist works, then triggers the failure (this is the same
      pattern used in the existing returns-whitelist.json test).
      
      Note that by adding this check, we have effectively forbidden
      an entity with a case-insensitive clash of member names, for
      any entity that is not on the whitelist (although there is
      still the possibility to clash via '-' vs. '_').
      
      Not done here: a future patch should also add naming convention
      support and whitelist exceptions for command, event, and type
      names.
      
      The additions to QAPISchemaMember.check_clash() check whether
      info['name'] is in the whitelist (the top-most entity name at
      the point 'info' tracks), rather than self.owner (the type,
      possibly implicit, that directly owns the member), because it
      is easier to maintain the whitelist by the names actually in
      the user's .json file, rather than worrying about the names
      of implicit types.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1449033659-25497-14-git-send-email-eblake@redhat.com>
      [Simplified a bit as per discussion with Eric]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      893e1f2c
    • E
      qapi: Simplify visiting of alternate types · 0426d53c
      Eric Blake 提交于
      Previously, working with alternates required two lookup arrays
      and some indirection: for type Foo, we created Foo_qtypes[]
      which maps each qtype to a value of the generated FooKind enum,
      then look up that value in FooKind_lookup[] like we do for other
      union types.
      
      This has a couple of subtle bugs.  First, the generator was
      creating a call with a parameter '(int *) &(*obj)->type' where
      type is an enum type; this is unsafe if the compiler chooses
      to store the enum type in a different size than int, where
      assigning through the wrong size pointer can corrupt data or
      cause a SIGBUS.
      
      Related bug, not not fixed in this patch: qapi-visit.py's
      gen_visit_enum() generates a cast of its enum * argument to
      int *. Marked FIXME.
      
      Second, since the values of the FooKind enum start at zero, all
      entries of the Foo_qtypes[] array that were not explicitly
      initialized will map to the same branch of the union as the
      first member of the alternate, rather than triggering a desired
      failure in visit_get_next_type().  Fortunately, the bug seldom
      bites; the very next thing the input visitor does is try to
      parse the incoming JSON with the wrong parser, which normally
      fails; the output visitor is not used with a C struct in that
      state, and the dealloc visitor has nothing to clean up (so
      there is no leak).
      
      However, the second bug IS observable in one case: parsing an
      integer causes unusual behavior in an alternate that contains
      at least a 'number' member but no 'int' member, because the
      'number' parser accepts QTYPE_QINT in addition to the expected
      QTYPE_QFLOAT (that is, since 'int' is not a member, the type
      QTYPE_QINT accidentally maps to FooKind 0; if this enum value
      is the 'number' branch the integer parses successfully, but if
      the 'number' branch is not first, some other branch tries to
      parse the integer and rejects it).  A later patch will worry
      about fixing alternates to always parse all inputs that a
      non-alternate 'number' would accept, for now this is still
      marked FIXME in the updated test-qmp-input-visitor.c, to
      merely point out that new undesired behavior of 'ans' matches
      the existing undesired behavior of 'asn'.
      
      This patch fixes the default-initialization bug by deleting the
      indirection, and modifying get_next_type() to directly assign a
      QTypeCode parameter.  This in turn fixes the type-casting bug,
      as we are no longer casting a pointer to enum to a questionable
      size. There is no longer a need to generate an implicit FooKind
      enum associated with the alternate type (since the QMP wire
      format never uses the stringized counterparts of the C union
      member names).  Since the updated visit_get_next_type() does not
      know which qtypes are expected, the generated visitor is
      modified to generate an error statement if an unexpected type is
      encountered.
      
      Callers now have to know the QTYPE_* mapping when looking at the
      discriminator; but so far, only the testsuite was even using the
      C struct of an alternate types.  I considered the possibility of
      keeping the internal enum FooKind, but initialized differently
      than most generated arrays, as in:
        typedef enum FooKind {
            FOO_KIND_A = QTYPE_QDICT,
            FOO_KIND_B = QTYPE_QINT,
        } FooKind;
      to create nicer aliases for knowing when to use foo->a or foo->b
      when inspecting foo->type; but it turned out to add too much
      complexity, especially without a client.
      
      There is a user-visible side effect to this change, but I
      consider it to be an improvement. Previously,
      the invalid QMP command:
        {"execute":"blockdev-add", "arguments":{"options":
          {"driver":"raw", "id":"a", "file":true}}}
      failed with:
        {"error": {"class": "GenericError",
          "desc": "Invalid parameter type for 'file', expected: QDict"}}
      (visit_get_next_type() succeeded, and the error comes from the
      visit_type_BlockdevOptions() expecting {}; there is no mention of
      the fact that a string would also work).  Now it fails with:
        {"error": {"class": "GenericError",
          "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
      (the error when the next type doesn't match any expected types for
      the overall alternate).
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      0426d53c
    • E
      qapi: Convert QType into QAPI built-in enum type · 7264f5c5
      Eric Blake 提交于
      What's more meta than using qapi to define qapi? :)
      
      Convert QType into a full-fledged[*] builtin qapi enum type, so
      that a subsequent patch can then use it as the discriminator
      type of qapi alternate types.  Fortunately, the judicious use of
      'prefix' in the qapi definition avoids churn to the spelling of
      the enum constants.
      
      To avoid circular definitions, we have to flip the order of
      inclusion between "qobject.h" vs. "qapi-types.h".  Back in commit
      28770e05, we had the latter include the former, so that we could
      use 'QObject *' for our implementation of 'any'.  But that usage
      also works with only a forward declaration, whereas the
      definition of QObject requires QType to be a complete type.
      
      [*] The type has to be builtin, rather than declared in
      qapi/common.json, because we want to use it for alternates even
      when common.json is not included. But since it is the first
      builtin enum type, we have to add special cases to qapi-types
      and qapi-visit to only emit definitions once, even when two
      qapi files are being compiled into the same binary (the way we
      already handled builtin list types like 'intList').  We may
      need to revisit how multiple qapi files share common types,
      but that's a project for another day.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1449033659-25497-4-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      7264f5c5
    • E
      qapi: Remove obsolete tests for MAX collision · 04e0639d
      Eric Blake 提交于
      Now that we no longer collide with an implicit _MAX enum member,
      we no longer need to reject it in the ad hoc parser, and can
      remove several tests that are no longer needed.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1447836791-369-24-git-send-email-eblake@redhat.com>
      [Commit message tweaked]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      04e0639d
    • E
      qapi: Tighten the regex on valid names · 59a92fee
      Eric Blake 提交于
      We already documented that qapi names should match specific
      patterns (such as starting with a letter unless it was an enum
      value or a downstream extension).  Tighten that from a suggestion
      into a hard requirement, which frees up names beginning with a
      single underscore for qapi internal usage.
      
      The tighter regex doesn't forbid everything insane that a user
      could provide (for example, a user could name a type 'Foo-lookup'
      to collide with the generated 'Foo_lookup[]' for an enum 'Foo'),
      but does a good job at protecting the most obvious uses, and
      also happens to reserve single leading underscore for later use.
      
      The handling of enum values starting with a digit is tricky:
      commit 9fb081e0 introduced a subtle bug by using c_name() on
      a munged value, which would allow an enum to include the
      member 'q-int' in spite of our reservation.  Furthermore,
      munging with a leading '_' would fail our tighter regex.  So
      fix it by only munging for leading digits (which are never
      ticklish in c_name()) and by using a different prefix (I
      picked 'D', although any letter should do).
      
      Add new tests, reserved-member-underscore and reserved-enum-q,
      to demonstrate the tighter checking.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1447836791-369-22-git-send-email-eblake@redhat.com>
      Message-Id: <1447883135-18020-1-git-send-email-eblake@redhat.com>
      [Eric's fixup squashed in]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      59a92fee
    • E
      qapi: Fix c_name() munging · c43567c1
      Eric Blake 提交于
      The method c_name() is supposed to do two different actions: munge
      '-' into '_', and add a 'q_' prefix to ticklish names.  But it did
      these steps out of order, making it possible to submit input that
      is not ticklish until after munging, where the output then lacked
      the desired prefix.
      
      The failure is exposed easily if you have a compiler that recognizes
      C11 keywords, and try to name a member '_Thread-local', as it would
      result in trying to compile the declaration 'uint64_t _Thread_local;'
      which is not valid.  However, this name violates our conventions
      (ultimately, want to enforce that no qapi names start with single
      underscore), so the test is slightly weaker by instead testing
      'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where
      wchar_t is only a typedef) but would fail with a C++ compiler (where
      it is a keyword).
      
      Fix things by reversing the order of actions within c_name().
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1447836791-369-18-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      c43567c1
    • E
      qapi: Detect collisions in C member names · 27b60ab9
      Eric Blake 提交于
      Detect attempts to declare two object members that would result
      in the same C member name, by keying the 'seen' dictionary off
      of the C name rather than the qapi name.  It also requires passing
      info through the check_clash() methods.
      
      This addresses a TODO and fixes the previously-broken
      args-name-clash test.  The resulting error message demonstrates
      the utility of the .describe() method added previously.  No change
      to generated code.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1447836791-369-17-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      27b60ab9
    • E
      qapi: Remove outdated tests related to QMP/branch collisions · 61a94661
      Eric Blake 提交于
      Now that branches are in a separate C namespace, we can remove
      the restrictions in the parser that claim a branch name would
      collide with QMP, and delete the negative tests that are no
      longer problematic.  A separate patch can then add positive
      tests to qapi-schema-test to test that any corner cases will
      compile correctly.
      
      This reverts the scripts/qapi.py portion of commit 7b2a5c2f,
      now that the assertions that it plugged are no longer possible.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1447836791-369-15-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      61a94661
    • E
      qapi: Track simple union tag in object.local_members · da34a9bd
      Eric Blake 提交于
      We were previously creating all unions with an empty list for
      local_members.  However, it will make it easier to unify struct
      and union generation if we include the generated tag member in
      local_members.  That way, we can have a common code pattern:
      visit the base (if any), visit the local members (if any), visit
      the variants (if any).  The local_members of a flat union
      remains empty (because the discriminator is already visited as
      part of the base).  Then, by visiting tag_member.check() during
      AlternateType.check(), we no longer need to call it during
      Variants.check().
      
      The various front end entities now exist as follows:
      struct: optional base, optional local_members, no variants
      simple union: no base, one-element local_members, variants with tag_member
        from local_members
      flat union: base, no local_members, variants with tag_member from base
      alternate: no base, no local_members, variants
      
      With the new local members, we require a bit of finesse to
      avoid assertions in the clients.
      
      No change to generated code.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1447836791-369-2-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      da34a9bd
  6. 09 11月, 2015 1 次提交
    • E
      qapi: Use generated TestStruct machinery in tests · 748053c9
      Eric Blake 提交于
      Commit d88f5fd1 and friends first introduced the various test-qmp-*
      tests in 2011, with duplicated hand-rolled TestStruct machinery,
      to make sure the qapi visitor interface was tested.  Later, commit
      4f193e34 in 2013 added a .json file for further testing use by the
      files, but without consolidating any of the existing hand-rolled
      visitors.  And with four copies, subtle differences have crept in,
      between the tests themselves (mainly whitespace differences, but
      also a question of whether to use NULL or "TestStruct" when
      calling visit_start_struct()) and from what the generator produces
      (the hand-rolled versions did not cater to partially-allocated
      objects, because they did not have a deallocation usage).
      
      Of course, just because the visitor interface is tested does not
      mean it is a sane interface; and future patches will be changing
      some of the visitor contracts.  Rather than having to duplicate
      the cleanup work in each copy of the TestStruct visitor, and keep
      each hand-rolled copy in sync with what the generator supplies, we
      might as well just test what the generator should give us in the
      first place.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1446791754-23823-2-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      748053c9
  7. 02 11月, 2015 5 次提交
    • E
      qapi: Reserve 'u' member name · 5e59baf9
      Eric Blake 提交于
      Now that we have separated union tag values from colliding with
      non-variant C names, by naming the union 'u', we should reserve
      this name for our use.  Note that we want to forbid 'u' even in
      a struct with no variants, because it is possible for a future
      qemu release to extend QMP in a backwards-compatible manner while
      converting from a struct to a flat union.  Fortunately, no
      existing clients were using this member name.  If we ever find
      the need for QMP to have a member 'u', we could at that time
      relax things, perhaps by having c_name() munge the QMP member to
      'q_u'.
      
      Note that we cannot forbid 'u' everywhere (by adding the
      rejection code to check_name()), because the existing QKeyCode
      enum already uses it; therefore we only reserve it as a struct
      type member name.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1445898903-12082-24-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      5e59baf9
    • E
      qapi: Unbox base members · ddf21908
      Eric Blake 提交于
      Rather than storing a base class as a pointer to a box, just
      store the fields of that base class in the same order, so that
      a child struct can be directly cast to its parent.  This gives
      less malloc overhead, less pointer dereferencing, and even less
      generated code.  Compare to the earlier commit 1e6c1616 "qapi:
      Generate a nicer struct for flat unions" (although that patch
      had fewer places to change, as less of qemu was directly using
      qapi structs for flat unions).  It also allows us to turn on
      automatic type-safe wrappers for upcasting to the base class
      of a struct.
      
      Changes to the generated code look like this in qapi-types.h:
      
      | struct SpiceChannel {
      |-    SpiceBasicInfo *base;
      |+    /* Members inherited from SpiceBasicInfo: */
      |+    char *host;
      |+    char *port;
      |+    NetworkAddressFamily family;
      |+    /* Own members: */
      |     int64_t connection_id;
      
      as well as additional upcast functions like qapi_SpiceChannel_base().
      Meanwhile, changes to qapi-visit.c look like:
      
      | static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp)
      | {
      |     Error *err = NULL;
      |
      |-    visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err);
      |+    visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo **)obj, &err);
      |     if (err) {
      
      (the cast is necessary, since our upcast wrappers only deal with a
      single pointer, not pointer-to-pointer); plus the wholesale
      elimination of some now-unused visit_type_implicit_FOO() functions.
      
      Without boxing, the corner case of one empty struct having
      another empty struct as its base type now requires inserting a
      dummy member (previously, the 'Base *base' member sufficed).
      
      And now that we no longer consume a 'base' member in the generated
      C struct, we can delete the former negative struct-base-clash-base
      test.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1445898903-12082-11-git-send-email-eblake@redhat.com>
      [Commit message tweaked slightly]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      ddf21908
    • E
      qapi: Reserve 'q_*' and 'has_*' member names · 9fb081e0
      Eric Blake 提交于
      c_name() produces names starting with 'q_' when protecting a
      dictionary member name that would fail to directly compile, but
      in doing so can cause clashes with any member name already
      beginning with 'q-' or 'q_'.  Likewise, we create a C name 'has_'
      for any optional member that can clash with any member name
      beginning with 'has-' or 'has_'.
      
      Technically, rather than blindly reserving the namespace,
      we could try to complain about user names only when an actual
      collision occurs, or even teach c_name() how to munge names
      to avoid collisions.  But it is not trivial, especially when
      collisions can occur across multiple types (such as via
      inheritance or flat unions).  Besides, no existing .json
      files are trying to use these names.  So it's easier to just
      outright forbid the potential for collision.  We can always
      relax things in the future if a real need arises for QMP to
      express member names that have been forbidden here.
      
      'has_' only has to be reserved for struct/union member names,
      while 'q_' is reserved everywhere (matching the fact that
      only members can be optional, while we use c_name() for munging
      both members and entities).  Note that we could relax 'q_'
      restrictions on entities independently from member names; for
      example, c_name('qmp_' + 'unix') would result in a different
      function name than our current 'qmp_' + c_name('unix').
      
      Update and add tests to cover the new error messages.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1445898903-12082-6-git-send-email-eblake@redhat.com>
      [Consistently pass protect=False to c_name(); commit message tweaked
      slightly]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      9fb081e0
    • E
      qapi: Reserve '*List' type names for list types · 255960dd
      Eric Blake 提交于
      Type names ending in 'List' can clash with qapi list types in
      generated C.  We don't currently use such names. It is easier to
      outlaw them now than to worry about how to resolve such a clash
      in the future. For precedence, see commit 4dc2e690, which did the
      same for names ending in 'Kind' versus implicit enum types for
      qapi unions.
      
      Update the testsuite to match.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1445898903-12082-5-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      255960dd
    • E
      tests/qapi-schema: Test for reserved names, empty struct · 19767083
      Eric Blake 提交于
      Add some testsuite coverage to ensure future patches are on
      the right track:
      
      Our current C representation of qapi arrays is done by appending
      'List' to the element name; but we are not preventing the
      creation of an object type with the same name.  Add
      reserved-type-list.json to test this.  Then rename
      enum-union-clash.json to reserved-type-kind.json to cover the
      reservation that we DO detect, and shorten it to match the fact
      that the name is reserved even if there is no clash.
      
      We are failing to detect a collision between a dictionary member
      and the implicit 'has_*' flag for another optional member. The
      easiest fix would be for a future patch to reserve the entire
      "has[-_]" namespace for member names (the collision is also
      possible for branch names within flat unions, but only as long as
      branch names can collide with (non-variant) members; however,
      since future patches are about to remove that, it is not worth
      testing here). Add reserved-member-has.json to test this.
      
      A similar collision exists between a dictionary member where
      c_name() munges what might otherwise be a reserved name to start
      with 'q_', and another member explicitly starts with "q[-_]".
      Again, the easiest solution for a future patch will be reserving
      the entire namespace, but here for commands as well as members.
      Add reserved-member-q.json and reserved-command-q.json to test
      this; separate tests since arguably our munging of command 'unix'
      to 'qmp_q_unix()' could be done without a q_, which is different
      than the munging of a member 'unix' to 'foo.q_unix'.
      
      Finally, our testsuite does not have any compilation coverage
      of struct inheritance with empty qapi structs.  Update
      qapi-schema-test.json to test this.
      
      Note that there is currently no technical reason to forbid type
      name patterns from member names, or member name patterns from
      types, since the two are not in the same namespace in C and
      won't collide; but it's not worth adding positive tests of these
      corner cases at this time, especially while there is other churn
      pending in patches that rearrange which collisions actually
      happen.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1445898903-12082-2-git-send-email-eblake@redhat.com>
      [Commit message tweaked slightly]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      19767083
  8. 15 10月, 2015 6 次提交
    • E
      qapi: Lazy creation of array types · 9f08c8ec
      Eric Blake 提交于
      Commit ac88219a had several TODO markers about whether we needed
      to automatically create the corresponding array type alongside
      any other type.  It turns out that most of the time, we don't!
      
      There are a few exceptions: 1) We have a few situations where we
      use an array type in internal code but do not expose that type
      through QMP; fix it by declaring a dummy type that forces the
      generator to see that we want to use the array type.
      
      2) The builtin arrays (such as intList for QAPI ['int']) must
      always be generated, because of the way our QAPI_TYPES_BUILTIN
      compile guard works: we have situations (at the very least
      tests/test-qmp-output-visitor.c) that include both top-level
      "qapi-types.h" (via "error.h") and a secondary
      "test-qapi-types.h". If we were to only emit the builtin types
      when used locally, then the first .h file would not include all
      types, but the second .h does not declare anything at all because
      the first .h set QAPI_TYPES_BUILTIN, and we would end up with
      compilation error due to things like unknown type 'int8List'.
      
      Actually, we may need to revisit how we do type guards, and
      change from a single QAPI_TYPES_BUILTIN over to a different
      usage pattern that does one #ifdef per qapi type - right now,
      the only types that are declared multiple times between two qapi
      .json files for inclusion by a single .c file happen to be the
      builtin arrays.  But now that we have QAPI 'include' statements,
      it is logical to assume that we will soon reach a point where
      we want to reuse non-builtin types (yes, I'm thinking about what
      it will take to add introspection to QGA, where we will want to
      reuse the SchemaInfo type and friends).  One #ifdef per type
      will help ensure that generating the same qapi type into more
      than one qapi-types.h won't cause collisions when both are
      included in the same .c file; but we also have to solve how to
      avoid creating duplicate qapi-types.c entry points.  So that
      is a problem left for another day.
      
      Generated code for qapi-types and qapi-visit is drastically
      reduced; less than a third of the arrays that were blindly
      created were actually needed (a quick grep shows we dropped
      from 219 to 69 *List types), and the .o files lost more than
      30% of their bulk.  [For best results, diff the generated
      files with 'git diff --patience --no-index pre post'.]
      
      Interestingly, the introspection output is unchanged - this is
      because we already cull all types that are not indirectly
      reachable from a command or event, so introspection was already
      using only a subset of array types.  The subset of types
      introspected is now a much larger percentage of the overall set
      of array types emitted in qapi-types.h (since the larger set
      shrunk), but still not 100% (evidence that the array types
      emitted for our new Dummy structs, and the new struct itself,
      don't affect QMP).
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1444710158-8723-9-git-send-email-eblake@redhat.com>
      [Moved array info tracking to a later patch]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      9f08c8ec
    • E
      qapi: Drop redundant args-member-array test · 849ab13c
      Eric Blake 提交于
      qapi-schema-test already ensures that we can correctly compile
      an array of enums (__org.qemu_x-command), an array of builtins
      (UserDefNativeListUnion), and an array of structs (again
      __org.qemu_x-command).  That means args-member-array is not
      adding any additional parse-only test coverage, so drop it.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1444760807-11307-1-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      849ab13c
    • E
      qapi: Drop redundant flat-union-reverse-define test · 70478cef
      Eric Blake 提交于
      As of commit 8c3f8e77, we test compilation of forward references
      for a struct base type (UserDefOne), flat union base type
      (UserDefUnionBase), and flat union branch type
      (UserDefFlatUnion2). The only remaining forward reference being
      tested for parsing in flat-union-reverse-define was a forward
      enum declaration.  Once we make sure that always compiles,
      the smaller parse-only test is redundant and can be deleted.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1444710158-8723-7-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      70478cef
    • E
      qapi: Drop redundant returns-int test · cae95eae
      Eric Blake 提交于
      qapi-schema-test was already testing that we could have a
      command returning int, but burned a command name in the whitelist.
      Merge the redundant positive test returns-int, and pick a name
      that reduces the whitelist size.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1444710158-8723-6-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      cae95eae
    • E
      qapi: Move empty-enum to compile-time test · 625b251c
      Eric Blake 提交于
      Rather than just asserting that we can parse an empty enum,
      let's also make sure we can compile it, by including it in
      qapi-schema-test.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1444710158-8723-5-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      625b251c
    • E
      qapi: Drop redundant alternate-good test · baabb84c
      Eric Blake 提交于
      The alternate-good.json test was already covered by
      qapi-schema-test.json.  As future commits will be tweaking
      how alternates are laid out, removing the duplicate test now
      reduces churn.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1444710158-8723-4-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      baabb84c
  9. 13 10月, 2015 4 次提交
    • E
      qapi: Reuse code for flat union base validation · 376863ef
      Eric Blake 提交于
      Rather than open-code the check for a valid base type, we
      should reuse the common functionality. This allows for
      consistent error messages, and also makes it easier for a
      later patch to turn on support for inline anonymous base
      structures.
      
      Test flat-union-inline is updated to test only one feature
      (anonymous branch dictionaries), which can be implemented
      independently (test flat-union-bad-base already covers the
      idea of an anonymous base dictionary).
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1443565276-4535-10-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      376863ef
    • E
      qapi: Test use of 'number' within alternates · 9c51b441
      Eric Blake 提交于
      Add some testsuite exposure for use of a 'number' as part of
      an alternate.  The current state of the tree has a few bugs
      exposed by this: our input parser depends on the ordering of
      how the qapi schema declared the alternate, and the parser
      does not accept integers for a 'number' in an alternate even
      though it does for numbers outside of an alternate.
      
      Mixing 'int' and 'number' in the same alternate is unusual,
      since both are supplied by json-numbers, but there does not
      seem to be a technical reason to forbid it given that our
      json lexer distinguishes between json-numbers that can be
      represented as an int vs. those that cannot.
      
      Improve the existing test_visitor_in_alternate() to match the
      style of the new test_visitor_in_alternate_number(), and to
      ensure full coverage of all possible qtype parsing.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1443565276-4535-9-git-send-email-eblake@redhat.com>
      [Eric's follow-up fixes squashed in]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      9c51b441
    • E
      qapi: Add tests for empty unions · 8d25dd10
      Eric Blake 提交于
      The documentation claims that alternates are useful for
      allowing two or more types, although nothing enforces this.
      Meanwhile, it is silent on whether empty unions are allowed.
      In practice, the generated code will compile, in part because
      we have a 'void *data' branch; but attempting to visit such a
      type will cause an abort().  While there's no technical reason
      that a degenerate union could not be made to work, it's harder
      to justify the time spent in chasing known (the current
      abort() during visit) and unknown corner cases, than it would
      be to just outlaw them.  A future patch will probably take the
      approach of forbidding them; in the meantime, we can at least
      add testsuite coverage to make it obvious where things stand.
      
      In addition to adding tests to expose the problems, we also
      need to adjust existing tests that are meant to test something
      else, but which could fail for the wrong reason if we reject
      degenerate alternates/unions.
      
      Note that empty structs are explicitly supported (for example,
      right now they are the only way to specify that one branch of a
      flat union adds no additional members), and empty enums are
      covered by the testsuite as working (even if they do not seem
      to have much use).
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1443565276-4535-8-git-send-email-eblake@redhat.com>
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      8d25dd10
    • E
      qapi: Avoid assertion failure on union 'type' collision · 7b2a5c2f
      Eric Blake 提交于
      The previous commit added two tests that triggered an assertion
      failure. It's fairly straightforward to avoid the failure by
      just outright forbidding the collision between a union's tag
      values and its discriminator name (including the implicit name
      'kind' supplied for simple unions [*]).  Ultimately, we'd like
      to move the collision detection into QAPISchema*.check(), but
      for now it is easier just to enhance the existing checks.
      
      [*] Of course, down the road, we have plans to rename the simple
      union tag name to 'type' to match the QMP wire name, but the
      idea of the collision will still be present even then.
      
      Technically, we could avoid the collision by naming the C union
      members representing each enum value as '_case_value' rather
      than 'value'; but until we have an actual qapi client (and not
      just our testsuite) that has a legitimate reason to match a
      case label to the name of a QMP key and needs the name munging
      to satisfy the compiler, it's easier to just reject the qapi
      as invalid.
      Signed-off-by: NEric Blake <eblake@redhat.com>
      Message-Id: <1443565276-4535-7-git-send-email-eblake@redhat.com>
      [Polished a few comments]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      7b2a5c2f