[v10,09/25] qapi: Prefer typesafe upcasts to qapi base classes

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

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 <eblake@redhat.com>

---
v10: new patch
---
 scripts/qapi-types.py | 20 ++++++++++++++++++++
 ui/spice-core.c       | 14 ++++++++++----
 ui/vnc.c              |  9 ++++++---
 3 files changed, 36 insertions(+), 7 deletions(-)

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>
> ---
> 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?

On 10/23/2015 09:30 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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> writes:

> On 10/23/2015 09:30 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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.

On 10/26/2015 01:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 10/23/2015 09:30 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> 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.
>>>>

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

It sounds like I have two options for v11:

1. Keep 9/25 introducing gen_upcast(), just for union types, and
including testsuite coverage. In 10/25, make use of the upcast functions
to struct as part of making structs sane.

2. Swap the patch order: do 10/25 to alter struct layout first, using
ugly casts; then implement 9/25 that adds gen_upcast() and fixes the
ugly casts to instead use the new upcast functions.

I can go either way, so do you have any preference?

Eric Blake <eblake@redhat.com> writes:

> On 10/26/2015 01:33 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 10/23/2015 09:30 AM, Markus Armbruster wrote:
>>>> Eric Blake <eblake@redhat.com> 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.
>>>>>
>
>>>
>>> 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.
>
> It sounds like I have two options for v11:
>
> 1. Keep 9/25 introducing gen_upcast(), just for union types, and
> including testsuite coverage. In 10/25, make use of the upcast functions
> to struct as part of making structs sane.
>
> 2. Swap the patch order: do 10/25 to alter struct layout first, using
> ugly casts; then implement 9/25 that adds gen_upcast() and fixes the
> ugly casts to instead use the new upcast functions.
>
> I can go either way, so do you have any preference?

I think I'd 3. Do 10/25 first, with gen_upcast() squashed in, and used
for casts to base.  That gen_upcast() will just work for unions, too.
Least churn.

But any of the three options should work.

On 10/26/2015 11:54 AM, Markus Armbruster wrote:

>> It sounds like I have two options for v11:
>>
>> 1. Keep 9/25 introducing gen_upcast(), just for union types, and
>> including testsuite coverage. In 10/25, make use of the upcast functions
>> to struct as part of making structs sane.
>>
>> 2. Swap the patch order: do 10/25 to alter struct layout first, using
>> ugly casts; then implement 9/25 that adds gen_upcast() and fixes the
>> ugly casts to instead use the new upcast functions.
>>
>> I can go either way, so do you have any preference?
> 
> I think I'd 3. Do 10/25 first, with gen_upcast() squashed in, and used
> for casts to base.  That gen_upcast() will just work for unions, too.
> Least churn.

I actually started with option 3 back when first writing v10, then
decided that it made the patch big enough, and the addition was
orthogonal enough, to go with the split patches at the time I actually
posted v10.

> 
> But any of the three options should work.

Good.  Because I had already gone with option 1 locally before asking
the question, which means now I don't have to respin to get option 2 or 3 :)