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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

I guess I'd introduce gen_upcast() only with its first real user,
i.e. when unboxing struct base.  At that time, its obvious
implementation should work for both struct and union, even though we
actually use it only for struct.  If you want to add a test case for
unions, go ahead.  I'm not sure I'd bother, because once we unify code
generation for the two, testing them separately won't add much value
anymore.