Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> BlockdevOptionsRbd member auth-supported is a list of struct
> RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
> reason for wrapping the enum in a struct.

Well, the previous patch removed the QemuOpts madness as a technical
reason that required the wrapper, leaving nothing else technically using
it at the moment.  But there's still the design to think about...

>  Delete the wrapper, and
> rename the enum.

This patch changes the QMP wire format. It MUST go in 2.9, otherwise,
we've baked in the insanity; that is, if we decide it is insanity [1]

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/rbd.c          |  2 +-
>  qapi/block-core.json | 15 ++-------------
>  2 files changed, 3 insertions(+), 14 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -2601,25 +2601,14 @@
>  
>  
>  ##
> -# @RbdAuthSupport:
> -#
> -# An enumeration of RBD auth support
> -#
> -# Since: 2.9
> -##
> -{ 'enum': 'RbdAuthSupport',
> -  'data': [ 'cephx', 'none' ] }
> -
> -
> -##
>  # @RbdAuthMethod:
>  #
>  # An enumeration of rados auth_supported types
>  #
>  # Since: 2.9
>  ##
> -{ 'struct': 'RbdAuthMethod',
> -  'data': { 'auth': 'RbdAuthSupport' } }
> +{ 'enum': 'RbdAuthMethod',
> +  'data': [ 'cephx', 'none' ] }

Changes BlockdevOptionsRbd from:

..., "auth-supported": [ { "auth": "none" } ],

to the potentially much nicer:

..., "auth-supported": [ "none" ],

[1] But what if we want to make auth-supported be an array of flat
unions, where 'auth' is the discriminator that determines if additional
members are needed?  In other words, the existing API would allow:

"auth-supported": [ { "auth": "cephx" },
                    { "auth": "new", "password-id": "foo" } ]

for specifying a new auth type 'new' that comes with an associated
'password-id' field for looking up a corresponding 'secret' object used
for retrieving the associated password when using that type of
authorization.  In that scenario, RbdAuthMethod would look more like
this pseudo-qapi:

{ 'union': 'RbdAuthMethod', 'base': { 'auth': 'RbdAuthSupport' },
  'discriminator': 'auth',
  'data': { 'none': {},
            'cephx': {},
            'new': { 'password-id': 'str' } } }

Assuming we are okay with not needing to extend the current one-member
RbdAuthMethod struct into a flat union, then this patch is fine, and you
can add:

Reviewed-by: Eric Blake <address@hidden>

But if you think the flat union expansion path may ever prove useful,
then maybe we don't want this patch after all.

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

signature.asc
Description: OpenPGP digital signature