Re: [Qemu-devel] [PATCH v10 09/25] qapi: Prefer typesafe upcasts to qapi base classes

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> A previous patch (commit 1e6c1616) made it possible to
> directly cast from a qapi type to its base type. A future
> patch will do likewise for structs.  However, it requires
> the client code to use a C cast, which turns off compiler
> type-safety checks if the client gets it wrong.  So this

Who's the client?  Suggest to simply drop "if the client gets it wrong".

> patch adds inline type-safe wrappers named qapi_FOO_base()
> for any type FOO that has a base, which can be used to
> upcast a qapi type to its base, then uses the new generated
> functions in places where we were already casting.
>
> Some of the ugliness of this patch will disappear once
> structs are laid out in the same manner as unions; mark
> it with TODO for now.
>
> Note that C makes const-correct upcasts annoying because
> it lacks overloads; these functions cast away const so that
> they can accept user pointers whether const or not, and the
> result in turn can be assigned to normal or const pointers.
> Alternatively, this could have been done with macros, but
> those tend to not have quite as much type safety.

Well, if you torture the preprocessor hard enough, it surrenders and
lets you do type checking.  Torture isn't pretty, though.  See for
instance

http://git.ozlabs.org/?p=ccan;a=blob;f=ccan/check_type/check_type.h;h=77501a95597c3e73396c270d54a8ed53a9defbc4;hb=HEAD

> This patch just adds upcasts.  None of our code needed to
> downcast from a base qapi class to a child.  Also, in the
> case of grandchildren (such as BlockdevOptionsQcow2), the
> caller will need to call two functions to get to the inner
> base (although it wouldn't be too hard to generate a
> qapi_FOO_base_base() if desired).  If a user changes qapi
> to alter the base class hierarchy, such as going from
> 'A -> C' to 'A -> B -> C', it will change the type of
> 'qapi_C_base()', and the compiler will point out the places
> that are affected by the new base.
>
> One alternative was proposed, but was deemed too ugly to use
> in practice: the generators could output redundant
> information using anonymous types:
> | struct Child {
> |     union {
> |         struct {
> |             Type1 parent_member1;
> |             Type2 parent_member2;
> |         };
> |         Parent base;
> |     };
> | };
> With that ugly proposal, for a given qapi type, obj->member
> and obj->base.member would refer to the same storage; allowing
> convenience in working with members without needing 'base.'
> allowing typesafe upcast without needing a C cast by accessing
> '&obj->base', and allowing downcasts from the parent back to
> the child possible through container_of(obj, Child, base).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch
> ---
>  scripts/qapi-types.py | 20 ++++++++++++++++++++
>  ui/spice-core.c       | 14 ++++++++++----
>  ui/vnc.c              |  9 ++++++---
>  3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index a97cc10..ed4a11c 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -97,6 +97,23 @@ struct %(c_name)s {
>      return ret
>
>
> +def gen_upcast(name, base, struct):
> +    # C makes const-correctness ugly.  We have to cast away const to let
> +    # this function work for both const and non-const obj.
> +    # TODO Ugly difference between struct and flat union bases
> +    member = ''
> +    if struct:
> +        member = '->base'
> +    return mcgen('''
> +
> +static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
> +{
> +    return (%(base)s *)obj%(member)s;

> +}
> +''',
> +                 c_name=c_name(name), base=base.c_name(), member=member)

The ugliness doesn't bother me much, because it'll go away.  But perhaps
we should simply limit gen_upcast() to unions now, and extend it to
structs when we unbox their base.

> +
> +
>  def gen_alternate_qtypes_decl(name):
>      return mcgen('''
>
> @@ -268,6 +285,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>              self.decl += gen_union(name, base, variants)
>          else:
>              self.decl += gen_struct(name, base, members)
> +        if base:
> +            # TODO Drop last param once structs have sane layout
> +            self.decl += gen_upcast(name, base, not variants)
>          self._gen_type_cleanup(name)
>
>      def visit_alternate_type(self, name, info, variants):
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index bf4fd07..d43cfbe 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -218,9 +218,11 @@ static void channel_event(int event, 
> SpiceChannelEventInfo *info)
>      }
>
>      if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
> -        add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
> +        add_addr_info(qapi_SpiceChannel_base(client),
> +                      (struct sockaddr *)&info->paddr_ext,
>                        info->plen_ext);
> -        add_addr_info(server->base, (struct sockaddr *)&info->laddr_ext,
> +        add_addr_info(qapi_SpiceServerInfo_base(server),
> +                      (struct sockaddr *)&info->laddr_ext,
>                        info->llen_ext);
>      } else {
>          error_report("spice: %s, extended address is expected",

This change will start to make sense when we unbox base.  Right now, it
doesn't :)

> @@ -229,7 +231,9 @@ static void channel_event(int event, 
> SpiceChannelEventInfo *info)
>
>      switch (event) {
>      case SPICE_CHANNEL_EVENT_CONNECTED:
> -        qapi_event_send_spice_connected(server->base, client->base, 
> &error_abort);
> +        qapi_event_send_spice_connected(qapi_SpiceServerInfo_base(server),
> +                                        qapi_SpiceChannel_base(client),
> +                                        &error_abort);
>          break;
>      case SPICE_CHANNEL_EVENT_INITIALIZED:
>          if (auth) {
> @@ -242,7 +246,9 @@ static void channel_event(int event, 
> SpiceChannelEventInfo *info)
>          break;
>      case SPICE_CHANNEL_EVENT_DISCONNECTED:
>          channel_list_del(info);
> -        qapi_event_send_spice_disconnected(server->base, client->base, 
> &error_abort);
> +        qapi_event_send_spice_disconnected(qapi_SpiceServerInfo_base(server),
> +                                           qapi_SpiceChannel_base(client),
> +                                           &error_abort);
>          break;
>      default:
>          break;
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 502a10a..9473b32 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -263,7 +263,8 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
>
>      info = g_malloc(sizeof(*info));
>      info->base = g_malloc(sizeof(*info->base));
> -    vnc_init_basic_info_from_server_addr(vd->lsock, info->base, &err);
> +    vnc_init_basic_info_from_server_addr(vd->lsock,
> +                                         qapi_VncServerInfo_base(info), 
> &err);
>      info->has_auth = true;
>      info->auth = g_strdup(vnc_auth_name(vd));
>      if (err) {
> @@ -301,7 +302,8 @@ static void vnc_client_cache_addr(VncState *client)
>
>      client->info = g_malloc0(sizeof(*client->info));
>      client->info->base = g_malloc0(sizeof(*client->info->base));
> -    vnc_init_basic_info_from_remote_addr(client->csock, client->info->base,
> +    vnc_init_basic_info_from_remote_addr(client->csock,
> +                                         
> qapi_VncClientInfo_base(client->info),
>                                           &err);
>      if (err) {
>          qapi_free_VncClientInfo(client->info);
> @@ -326,7 +328,8 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
>
>      switch (event) {
>      case QAPI_EVENT_VNC_CONNECTED:
> -        qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
> +        qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info),
> +                                      &error_abort);
>          break;
>      case QAPI_EVENT_VNC_INITIALIZED:
>          qapi_event_send_vnc_initialized(si, vs->info, &error_abort);

Not a single cast to union base?