Re: [Qemu-devel] [PULL 14/15] qapi: Don't box branches of flat unions

[Daniel P. Berrange]

On Fri, Feb 19, 2016 at 01:18:05PM +0100, Markus Armbruster wrote:
> From: Eric Blake <address@hidden>
> 
> 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: Eric Blake <address@hidden>
> Message-Id: <address@hidden>
> [Dead code deletion squashed in, commit message updated accordingly]
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  cpus.c                          | 18 ++++++------------
>  hmp.c                           | 12 ++++++------
>  include/qapi/visitor-impl.h     |  2 --
>  include/qapi/visitor.h          |  1 -
>  qapi/qapi-dealloc-visitor.c     | 26 --------------------------
>  qapi/qapi-visit-core.c          |  8 --------
>  scripts/qapi-types.py           | 13 +++----------
>  scripts/qapi-visit.py           | 36 ++----------------------------------
>  tests/test-qmp-input-visitor.c  | 10 +++++-----
>  tests/test-qmp-output-visitor.c |  6 ++----
>  10 files changed, 24 insertions(+), 108 deletions(-)

This change has broken my pending LUKS encryption patch series[1], and
I'm not entirely sure of the best way to resolve it.

I'll simplify the schema slighty for sake of illustration here.
Consider I have the following:

  { 'struct': 'QCryptoBlockOptionsBase',
    'data': { 'format': 'QCryptoBlockFormat' }}

  { 'struct': 'QCryptoBlockOptionsQCow',
    'data': { '*key-secret': 'str' }}

  { 'struct': 'QCryptoBlockOptionsLUKS',
    'data': { '*key-secret': 'str',
              '*cipher-alg': 'QCryptoCipherAlgorithm',
              '*cipher-mode': 'QCryptoCipherMode',
              '*ivgen-alg': 'QCryptoIVGenAlgorithm',
              '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
              '*hash-alg': 'QCryptoHashAlgorithm'}}

  { 'union': 'QCryptoBlockOptions',
    'base': 'QCryptoBlockOptionsBase',
    'discriminator': 'format',
    'data': { 'qcow': 'QCryptoBlockOptionsQCow',
              'luks': 'QCryptoBlockOptionsLUKS' } }

If I want to visit QCryptoBlockOptions, allocating a new struct
instance, I can still do so

   QCryptoBlockOptions *opts = NULL;

   visit_type_QCryptoBlockOptions(v, "block", &opts, errp)


Similarly, if I want to visit QCryptoBlockOptionsLUKS, allocating
a new struct instance, I can still do so

   QCryptoBlockOptionsLUKS *opts = NULL;

   visit_type_QCryptoBlockOptionsLUKS(v, "luks", &opts, errp)

My code needs todo something a little different though - it needs
to directly visit one of the union branches. Previously, I could
allocate a QCryptoBlockOptions and then visit a specific union
branch like this:

   QCryptoBlockOptions *opts = NULL;

   opts = g_new0(QCryptoBlockOptions, 1);
   opts.format = Q_CRYPTO_BLOCK_FORMAT_LUKS

   visit_type_QCryptoBlockOptionsLUKS(v, "luks", &opts.u.luks, errp)


I need todo this, because my visitor instance does not have any
'format' field to visit - we know the format upfront from the
block layer driver choice. So we need to directly visit the
appropriate inline union branch. This no longer works, because
the opts.u.luks field is now inlined instead of being boxed :-(

I could visit_type_QCryptoBlockOptionsLUKS_fields(v, opts.u.luks, errp)
but the '_fields' methods are all declared static in qapi-visit.c
preventing their use.

IMHO, now that QAPI inlines the flat unions, we should be making
the _fields() methods public.

Regards,
Daniel

[1] https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03176.html
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|