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

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Feb 23, 2016 at 08:18:55AM -0700, Eric Blake wrote:
> On 02/23/2016 07:50 AM, Daniel P. Berrange wrote:
> > On Fri, Feb 19, 2016 at 01:18:05PM +0100, Markus Armbruster wrote:
> >> From: Eric Blake <eblake@redhat.com>
> >>
> >> 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.
> >>
> 
> > 
> > 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)
> 
> which allocated a pointer and visited the fields.  We no longer need the
> pointer, but still need the fields visited.
> 
> > 
> > 
> > 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 :-(
> 
> You're using OptsVisitor, right? Is it possible to hack the QemuOpts
> that you feed to opts_visitor_new() to include a 'format' field?

Yes, using QemuOpts, so I could feed in a fake 'format' i guess.

The temp hack I chose was to use an intermediate variable and then
memcpy & free. ie

    QCryptoBlockOpenOptions *ret;
    QCryptoBlockOptionsLUKS *tmp;

    ret = g_new0(QCryptoBlockOpenOptions, 1);
    ret->format = format;

    visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov), "luks",
                                       &tmp, &local_err);
    if (!local_err) {
        memcpy(&ret->u.luks, tmp, sizeof(*tmp));
        g_free(tmp);
    }

> > 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.
> 
> But my suggestion would just be a hack. Yours makes more sense in the
> long run, so I'll go ahead and propose that patch.

Regards,
Daniel
-- 
|: 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 :|