On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote: > Legacy -drive supports "password-secret" parameter that isn't > available with -blockdev / blockdev-add. That's because we backed out > our first try to provide it there due to interface design doubts, in > commit 577d8c9a811, v2.9.0. > > This is the second try. It brings back the parameter, except it's > named "key-secret" now. > > Let's review our reasons for backing out the first try, as stated in > the commit message: > > * BlockdevOptionsRbd member @password-secret isn't actually a > password, it's a key generated by Ceph.
I thought about that when I first added password-secret, but felt that it is still effectively acting as a password to authenticate to the server, and calling it password-secret made it clearer that it was related to the authentication phase, and not for example, disk encryption. > > Addressed by the rename. > > * We're not sure where member @password-secret belongs (see the > previous commit). > > See previous commit. > > * How @password-secret interacts with settings from a configuration > file specified with @conf is undocumented. > > Not actually true, the documentation for @conf says "Values in the > configuration file will be overridden by options specified via QAPI", > and we've tested this. > > Signed-off-by: Markus Armbruster <arm...@redhat.com> > --- > block/rbd.c | 41 +++++++++++++++++++++++++---------------- > qapi/block-core.json | 6 ++++++ > 2 files changed, 31 insertions(+), 16 deletions(-) > > diff --git a/block/rbd.c b/block/rbd.c > index ea0575d068..f2c6965418 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState > *bs, Error **errp) > } > > > -static int qemu_rbd_set_auth(rados_t cluster, const char *secretid, > - BlockdevOptionsRbd *opts, > +static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts, > Error **errp) > { > - char *acr; > + char *key, *acr; > int r; > GString *accu; > RbdAuthModeList *auth; > > - if (secretid) { > - gchar *secret = qcrypto_secret_lookup_as_base64(secretid, > - errp); > - if (!secret) { > - return -1; > + if (opts->key_secret) { > + key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp); > + if (!key) { > + return -EIO; > + } > + r = rados_conf_set(cluster, "key", key); > + g_free(key); > + if (r < 0) { > + error_setg_errno(errp, -r, "Could not set 'key'"); > + return r; > } > - > - rados_conf_set(cluster, "key", secret); > - g_free(secret); > } > > if (opts->has_auth_client_required) { > @@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = { > }, > }; > > -/* FIXME Deprecate and remove keypairs or make it available in QMP. > - * password_secret should eventually be configurable in opts->location. > Support > - * for it in .bdrv_open will make it work here as well. */ > +/* FIXME Deprecate and remove keypairs or make it available in QMP. */ > static int qemu_rbd_do_create(BlockdevCreateOptions *options, > const char *keypairs, const char > *password_secret, > Error **errp) > @@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, > rados_ioctx_t *io_ctx, > Error *local_err = NULL; > int r; > > + if (secretid) { > + if (opts->key_secret) { > + error_setg(errp, > + "Legacy 'password-secret' clashes with 'key-secret'"); > + return -EINVAL; > + } > + opts->key_secret = g_strdup(secretid); > + opts->has_key_secret = true; > + } > + > mon_host = qemu_rbd_mon_host(opts, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, > rados_ioctx_t *io_ctx, > } > } > > - if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) { > - r = -EIO; > + r = qemu_rbd_set_auth(*cluster, opts, errp); > + if (r < 0) { > goto failed_shutdown; > } > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 841d196a21..3eb536de5b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3120,6 +3120,11 @@ > # This maps to Ceph configuration option > # "auth_client_required". (Since 3.0) > # > +# @key-secret: ID of a QCryptoSecret object providing a key > +# for cephx authentication. > +# This maps to Ceph configuration option > +# "key". (Since 3.0) > +# > # @server: Monitor host address and port. This maps > # to the "mon_host" Ceph option. > # > @@ -3132,6 +3137,7 @@ > '*snapshot': 'str', > '*user': 'str', > '*auth-client-required': ['RbdAuthMode'], > + '*key-secret': 'str', > '*server': ['InetSocketAddressBase'] } } > > ## > -- > 2.17.1 > > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|