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

[Eric Blake]

On 10/23/2015 09:30 AM, Markus Armbruster wrote:
> 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

Oddly enough, our container_of() macro looks verrrrry similar to the
comments in that sample code (that is, we DO use the preprocessor for a
type check).  :)


>> +++ 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.
> 

Okay, I could do that.  Then I definitely need to post a v11 respin,
this is starting to be too much for simple fixups to v10.


>> -        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 :)

Indeed, if I don't port gen_upcast() to types until patch 10/25, then
this hunk also has to move to patch 10.  That means no clients of the
upcast macros in patch 9/25, _unless_ I add testsuite coverage.  Which I
probably ought to do.

>>      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?

Not that I could find. So I'll have to create one in the testsuite.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature