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

[Jeff Cody]

On Mon, Feb 27, 2017 at 04:47:54PM -0600, Eric Blake wrote:
> 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 '-'
> 

OK, thanks.

> > 
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  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.
> 
> 

OK

> > @@ -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 ':')?
> 

Good question - it looks like it can be enabled in the conf file at least,
from this: 

http://docs.ceph.com/docs/master/install/manual-deployment/


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

OK, I'll make it an enum (and fix the space, of course).

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

Yep - working on that now.  Thanks!

-Jeff