Re: [Qemu-devel] [PATCH v2 5/5] block/rbd: add support for 'mon_host', 'auth_supported' via QAPI

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

On 02/27/2017 12:58 PM, Jeff Cody wrote:
> This adds support for two additional options that may be specified
> by QAPI in blockdev-add:
> 
>     mon_host: servername and port
>     auth_supported: either 'cephx' or 'none'

Please spell new options with '-'

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c          | 39 +++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  8 ++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index e04a5e1..51e971e 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -394,6 +394,18 @@ static QemuOptsList runtime_opts = {
>              .name = "keyvalue-pairs",
>              .type = QEMU_OPT_STRING,
>          },
> +        {
> +            .name = "server.host",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "server.port",
> +            .type = QEMU_OPT_STRING,
> +        },

See Dan's comment about supporting more than one server via an array in
QAPI.

> +        {
> +            .name = "auth_supported",

Should be auth-supported in QAPI.


> @@ -604,6 +620,29 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_shutdown;
>      }
>  
> +    /* if mon_host was specified */
> +    if (host) {
> +        const char *hostname = host;
> +        char *mon_host = NULL;
> +
> +        if (port) {
> +            mon_host = g_strdup_printf("%s:%s", host, port);

Does Ceph care about IPv6 (in which case you may need [host]:port when
host itself includes ':')?

> +            hostname = mon_host;
> +        }
> +        r = rados_conf_set(s->cluster, "mon_host", hostname);
> +        g_free(mon_host);
> +        if (r < 0) {
> +            goto failed_shutdown;
> +        }
> +    }
> +
> +    if (auth_supported) {
> +        r = rados_conf_set(s->cluster, "auth_supported", auth_supported);

Translating QAPI auth-supported to rados auth_supported is fine.


> +++ b/qapi/block-core.json
> @@ -2680,6 +2680,12 @@
>  #
>  # @user:               #optional Ceph id name.
>  #
> +# @server:             #optional Monitor host address and port.  This maps
> +#                      to the "mon_host" Ceph option.
> +#
> +# @auth_supported:     #optional Authentication supported.
> +#                      Either "cephx" or"none".

Missing a space.

If you're going to support only a finite set of strings, this should be
a QAPI enum type, not 'str'.

> +#
>  # @password-secret:    #optional The ID of a QCryptoSecret object providing
>  #                       the password for the login.
>  #
> @@ -2691,6 +2697,8 @@
>              '*conf': 'str',
>              '*snapshot': 'str',
>              '*user': 'str',
> +            '*server': 'InetSocketAddress',
> +            '*auth_supported': 'str',
>              '*password-secret': 'str' } }
>  
>  ##
> 

Looks like we'll need a v3 to tweak this one.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]