Re: [PATCH 3/6] qapi: Remove wrapper struct for simple unions

[Kevin Wolf]

Am 23.10.2020 um 13:06 hat Marc-André Lureau geschrieben:
> On Fri, Oct 23, 2020 at 2:40 PM Marc-André Lureau 
> <marcandre.lureau@gmail.com> wrote:
> > On Fri, Oct 23, 2020 at 2:14 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> >> Variants of simple unions are always contained in a wrapper object
> >> called 'data' that serves no real use. When mapping a QAPI object to the
> >> command line, this becomes very visible to users because they have to
> >> add one or more useless 'data.' to many option names.
> >>
> >> As a first step towards avoiding this painful CLI experience, this gets
> >> rid of the 'data' indirection internally: The QAPI parser doesn't use a
> >> wrapper object as the variant type any more, so the indirection is
> >> removed from the generated C types. As a result, the C type looks the
> >> same for flat and simple unions now. A large part of this patch is
> >> mechanical conversion of C code to remove the 'data' indirection.
> >>
> >> Conceptually, the important change is that variants can now have flat
> >> and wrapped representations. For a transitioning period, we'll allow
> >> variants to support both representations in a later patch. Eventually,
> >> the plan is to deprecate and remove wrapped representations entirely,
> >> unifying simple and flat unions.
> >>
> >> The externally visible interfaces stay unchanged: Visitors still expect
> >> the 'data' wrappers, and introspection still shows it.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >
> > breaks the build for me:

Oops, apparently I'm not compiling spice code. I've installed the
libraries now and will fix this for v2 (it's just the same mechanical
conversion as elsewhere).

See below for the changes I'm squashing in.

> Other than that, I like the change, and it looks quite
> straightforward. I hope it's acceptable and we are not missing the
> reasons that extra data indirection was necessary.
> 
> Having to support both flat and wrapped versions in the externally visible
> interfaces might be tricky though.

It's actually pretty simple, I have some quickly hacked up draft patches
because Markus wanted to see what the path is for eventually getting rid
of the wrapped versions (and with that, the difference between simple
and flat unions).

Essentially what I'm doing is calling visit_optional("data") and if it's
there I do the visit_start/end_struct thing for wrapped representation,
and if not, it's flat.

This would be problematic if we had a simple union with a variant that
contains an option "data" (because then it would be ambiguous), but in
this case we're lucky for a change.

After that, it's setting the "deprecated" feature on the "data" member,
waiting for two releases and eventually removing the support for wrapped
representation.

I'm expecting the first part to happen in the 6.0 release cycle, and the
removal could be done in 6.2 then.

Kevin


diff --git a/chardev/spice.c b/chardev/spice.c
index 1104426e3a..8d8502dca7 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -258,7 +258,7 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
                                     bool *be_opened,
                                     Error **errp)
 {
-    ChardevSpiceChannel *spicevmc = backend->u.spicevmc.data;
+    ChardevSpiceChannel *spicevmc = &backend->u.spicevmc;
     const char *type = spicevmc->type;
     const char **psubtype = spice_server_char_device_recognized_subtypes();

@@ -294,7 +294,7 @@ static void qemu_chr_open_spice_port(Chardev *chr,
                                      bool *be_opened,
                                      Error **errp)
 {
-    ChardevSpicePort *spiceport = backend->u.spiceport.data;
+    ChardevSpicePort *spiceport = &backend->u.spiceport;
     const char *name = spiceport->fqdn;
     SpiceChardev *s;

@@ -321,14 +321,13 @@ static void qemu_chr_parse_spice_vmc(QemuOpts *opts, 
ChardevBackend *backend,
                                      Error **errp)
 {
     const char *name = qemu_opt_get(opts, "name");
-    ChardevSpiceChannel *spicevmc;
+    ChardevSpiceChannel *spicevmc = &backend->u.spicevmc;

     if (name == NULL) {
         error_setg(errp, "chardev: spice channel: no name given");
         return;
     }
     backend->type = CHARDEV_BACKEND_KIND_SPICEVMC;
-    spicevmc = backend->u.spicevmc.data = g_new0(ChardevSpiceChannel, 1);
     qemu_chr_parse_common(opts, qapi_ChardevSpiceChannel_base(spicevmc));
     spicevmc->type = g_strdup(name);
 }
@@ -337,14 +336,13 @@ static void qemu_chr_parse_spice_port(QemuOpts *opts, 
ChardevBackend *backend,
                                       Error **errp)
 {
     const char *name = qemu_opt_get(opts, "name");
-    ChardevSpicePort *spiceport;
+    ChardevSpicePort *spiceport = &backend->u.spiceport;
 
     if (name == NULL) {
         error_setg(errp, "chardev: spice port: no name given");
         return;
     }
     backend->type = CHARDEV_BACKEND_KIND_SPICEPORT;
-    spiceport = backend->u.spiceport.data = g_new0(ChardevSpicePort, 1);
     qemu_chr_parse_common(opts, qapi_ChardevSpicePort_base(spiceport));
     spiceport->fqdn = g_strdup(name);
 }
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 026124ef56..b542e927e5 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -60,7 +60,6 @@ chr_spice_backend_new(void)
     ChardevBackend *be = g_new0(ChardevBackend, 1);
 
     be->type = CHARDEV_BACKEND_KIND_SPICEPORT;
-    be->u.spiceport.data = g_new0(ChardevSpicePort, 1);
 
     return be;
 }
@@ -83,7 +82,7 @@ static void vc_chr_open(Chardev *chr,
     }
 
     be = chr_spice_backend_new();
-    be->u.spiceport.data->fqdn = fqdn ?
+    be->u.spiceport.fqdn = fqdn ?
         g_strdup(fqdn) : g_strdup_printf("org.qemu.console.%s", chr->label);
     vc->parent_open(chr, be, be_opened, errp);
     qapi_free_ChardevBackend(be);
@@ -182,7 +181,7 @@ static void spice_app_display_init(DisplayState *ds, 
DisplayOptions *opts)
     GError *err = NULL;
     gchar *uri;
 
-    be->u.spiceport.data->fqdn = g_strdup("org.qemu.monitor.qmp.0");
+    be->u.spiceport.fqdn = g_strdup("org.qemu.monitor.qmp.0");
     qemu_chardev_new("org.qemu.monitor.qmp", TYPE_CHARDEV_SPICEPORT,
                      be, NULL, &error_abort);
     qopts = qemu_opts_create(qemu_find_opts("mon"),