This reverts half of commit 0a55679. We're having second thoughts on
the QAPI schema (and thus the external interface), and haven't reached
consensus, yet. Issues include:
* The implementation uses deprecated rados_conf_set() key
"auth_supported". No biggie.
* The implementation makes -drive silently ignore invalid parameters
"auth" and "auth-supported.*.X" where X isn't "auth". Fixable (in
fact I'm going to fix similar bugs around parameter server), so
again no biggie.
* BlockdevOptionsRbd member @password-secret applies only to
authentication method cephx. Should it be a variant member of
RbdAuthMethod?
* BlockdevOptionsRbd member @user could apply to both methods cephx
and none, but I'm not sure it's actually used with none. If it
isn't, should it be a variant member of RbdAuthMethod?
* The client offers a *set* of authentication methods, not a list.
Should the methods be optional members of BlockdevOptionsRbd instead
of members of list @auth-supported? The latter begs the question
what multiple entries for the same method mean. Trivial question
now that RbdAuthMethod contains nothing but @type, but less so when
RbdAuthMethod acquires other members, such the ones discussed above.
* How BlockdevOptionsRbd member @auth-supported interacts with
settings from a configuration file specified with @conf is
undocumented. I suspect it's untested, too.
Let's avoid painting ourselves into a corner now, and revert the
feature for 2.9.
Note that users can still configure authentication methods with a
configuration file. They probably do that anyway if they use Ceph
outside QEMU as well.
qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
which is silly. This will be cleaned up shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/rbd.c | 31 +++----------------------------
qapi/block-core.json | 24 ------------------------
2 files changed, 3 insertions(+), 52 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index cf0bab0..103ce44 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -320,8 +320,7 @@ static QemuOptsList runtime_opts = {
.help = "Rados id name",
},
/*
- * server.* and auth-supported.* extracted manually, see
- * qemu_rbd_array_opts()
+ * server.* extracted manually, see qemu_rbd_array_opts()
*/
{
.name = "password-secret",
@@ -356,11 +355,6 @@ static QemuOptsList runtime_opts = {
.name = "port",
.type = QEMU_OPT_STRING,
},
- {
- .name = "auth",
- .type = QEMU_OPT_STRING,
- .help = "Supported authentication method, either cephx or none",
- },
{ /* end of list */ }
},
};
@@ -512,7 +506,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
}
#define RBD_MON_HOST 0
-#define RBD_AUTH_SUPPORTED 1
static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
Error **errp)
@@ -527,7 +520,7 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
Error *local_err = NULL;
int i;
- assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
+ assert(type == RBD_MON_HOST);
num_entries = qdict_array_entries(options, prefix);
@@ -573,10 +566,9 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
value = strbuf;
}
} else {
- value = qemu_opt_get(opts, "auth");
+ abort();
}
-
/* each iteration in the for loop will build upon the string, and if
* rados_str is NULL then it is our first pass */
if (rados_str) {
@@ -608,7 +600,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
QemuOpts *opts;
Error *local_err = NULL;
char *mon_host = NULL;
- char *auth_supported = NULL;
int r;
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -619,14 +610,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
- auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
- RBD_AUTH_SUPPORTED, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- r = -EINVAL;
- goto failed_opts;
- }
-
mon_host = qemu_rbd_array_opts(options, "server.",
RBD_MON_HOST, &local_err);
if (local_err) {
@@ -678,13 +661,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
}
}
- if (auth_supported) {
- r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
- if (r < 0) {
- goto failed_shutdown;
- }
- }
-
if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
r = -EIO;
goto failed_shutdown;
@@ -735,7 +711,6 @@ failed_shutdown:
failed_opts:
qemu_opts_del(opts);
g_free(mon_host);
- g_free(auth_supported);
return r;
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5d2efe4..6a7ca0b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2601,27 +2601,6 @@
##
-# @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' } }
-
-##
# @BlockdevOptionsRbd:
#
# @pool: Ceph pool name.
@@ -2639,8 +2618,6 @@
# @server: Monitor host address and port. This maps
# to the "mon_host" Ceph option.
#
-# @auth-supported: Authentication supported.
-#
# @password-secret: The ID of a QCryptoSecret object providing
# the password for the login.
#
@@ -2653,7 +2630,6 @@
'*snapshot': 'str',
'*user': 'str',
'*server': ['InetSocketAddressBase'],
- '*auth-supported': ['RbdAuthMethod'],
'*password-secret': 'str' } }
##
--
2.7.4
On 27.03.2017 15:26, Markus Armbruster wrote: > This reverts half of commit 0a55679. We're having second thoughts on > the QAPI schema (and thus the external interface), and haven't reached > consensus, yet. Issues include: > > * The implementation uses deprecated rados_conf_set() key > "auth_supported". No biggie. > > * The implementation makes -drive silently ignore invalid parameters > "auth" and "auth-supported.*.X" where X isn't "auth". Fixable (in > fact I'm going to fix similar bugs around parameter server), so > again no biggie. > > * BlockdevOptionsRbd member @password-secret applies only to > authentication method cephx. Should it be a variant member of > RbdAuthMethod? > > * BlockdevOptionsRbd member @user could apply to both methods cephx > and none, but I'm not sure it's actually used with none. If it > isn't, should it be a variant member of RbdAuthMethod? > > * The client offers a *set* of authentication methods, not a list. > Should the methods be optional members of BlockdevOptionsRbd instead > of members of list @auth-supported? The latter begs the question > what multiple entries for the same method mean. Trivial question > now that RbdAuthMethod contains nothing but @type, but less so when > RbdAuthMethod acquires other members, such the ones discussed above. > > * How BlockdevOptionsRbd member @auth-supported interacts with > settings from a configuration file specified with @conf is > undocumented. I suspect it's untested, too. > > Let's avoid painting ourselves into a corner now, and revert the > feature for 2.9. > > Note that users can still configure authentication methods with a > configuration file. They probably do that anyway if they use Ceph > outside QEMU as well. > > qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST, > which is silly. This will be cleaned up shortly. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/rbd.c | 31 +++---------------------------- > qapi/block-core.json | 24 ------------------------ > 2 files changed, 3 insertions(+), 52 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 03/27/2017 08:26 AM, Markus Armbruster wrote:
> This reverts half of commit 0a55679. We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet. Issues include:
>
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
>
> Note that users can still configure authentication methods with a
> configuration file. They probably do that anyway if they use Ceph
> outside QEMU as well.
If we're only reverting the QMP blockdev-add feature, then this makes
absolute sense (it's not a regression since we don't have a release with
it yet, and we don't want to bake something into the release that can't
be supported). But breaking -drive usage seems risky, especially since
libvirt is already expecting to work - I'm worried that doing this may
break existing libvirt command line usage if the QemuOpts side doesn't
permit anything at all. Maybe we need to rely on your '=foo' or 'x-foo'
hack for letting QemuOpts still accept the old spelling during -drive
but not during QMP.
>
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly. This will be cleaned up shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/rbd.c | 31 +++----------------------------
> qapi/block-core.json | 24 ------------------------
> 2 files changed, 3 insertions(+), 52 deletions(-)
>
> +++ b/qapi/block-core.json
> @@ -2601,27 +2601,6 @@
>
>
> ##
> -# @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' } }
> -
Removing the .json QMP support is fine. But I'm reluctant to give R-b
without knowing for sure that -drive usage won't regress.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
On 03/27/2017 08:26 AM, Markus Armbruster wrote: > This reverts half of commit 0a55679. We're having second thoughts on > the QAPI schema (and thus the external interface), and haven't reached > consensus, yet. Issues include: > > Let's avoid painting ourselves into a corner now, and revert the > feature for 2.9. There may still be some tweaks to improve the commit message and/or code comments to clarify things that tripped me up until later in the subthread, but now that I understand the difference between pseudo-file format (where the key-value pair backdoor still works for libvirt's usage of -drive file=rbd:...) and QemuOpts format (-drive driver=rbd,... which didn't really exist in 2.8, and where we don't want to bake in something we don't like in 2.9), I agree with the move. > > Note that users can still configure authentication methods with a > configuration file. They probably do that anyway if they use Ceph > outside QEMU as well. > > qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST, > which is silly. This will be cleaned up shortly. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/rbd.c | 31 +++---------------------------- > qapi/block-core.json | 24 ------------------------ > 2 files changed, 3 insertions(+), 52 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Mar 27, 2017 at 03:26:32PM +0200, Markus Armbruster wrote:
> This reverts half of commit 0a55679. We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet. Issues include:
>
> * The implementation uses deprecated rados_conf_set() key
> "auth_supported". No biggie.
>
> * The implementation makes -drive silently ignore invalid parameters
> "auth" and "auth-supported.*.X" where X isn't "auth". Fixable (in
> fact I'm going to fix similar bugs around parameter server), so
> again no biggie.
>
> * BlockdevOptionsRbd member @password-secret applies only to
> authentication method cephx. Should it be a variant member of
> RbdAuthMethod?
>
> * BlockdevOptionsRbd member @user could apply to both methods cephx
> and none, but I'm not sure it's actually used with none. If it
> isn't, should it be a variant member of RbdAuthMethod?
>
> * The client offers a *set* of authentication methods, not a list.
> Should the methods be optional members of BlockdevOptionsRbd instead
> of members of list @auth-supported? The latter begs the question
> what multiple entries for the same method mean. Trivial question
> now that RbdAuthMethod contains nothing but @type, but less so when
> RbdAuthMethod acquires other members, such the ones discussed above.
>
> * How BlockdevOptionsRbd member @auth-supported interacts with
> settings from a configuration file specified with @conf is
> undocumented. I suspect it's untested, too.
>
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
>
> Note that users can still configure authentication methods with a
> configuration file. They probably do that anyway if they use Ceph
> outside QEMU as well.
>
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly. This will be cleaned up shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
I think this move makes sense; it allows blockdev-add to still be supported
for rbd, but does not lock us into a perhaps unwieldy API.
Reviewed-by: Jeff Cody <jcody@redhat.com>
> ---
> block/rbd.c | 31 +++----------------------------
> qapi/block-core.json | 24 ------------------------
> 2 files changed, 3 insertions(+), 52 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index cf0bab0..103ce44 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -320,8 +320,7 @@ static QemuOptsList runtime_opts = {
> .help = "Rados id name",
> },
> /*
> - * server.* and auth-supported.* extracted manually, see
> - * qemu_rbd_array_opts()
> + * server.* extracted manually, see qemu_rbd_array_opts()
> */
> {
> .name = "password-secret",
> @@ -356,11 +355,6 @@ static QemuOptsList runtime_opts = {
> .name = "port",
> .type = QEMU_OPT_STRING,
> },
> - {
> - .name = "auth",
> - .type = QEMU_OPT_STRING,
> - .help = "Supported authentication method, either cephx or none",
> - },
> { /* end of list */ }
> },
> };
> @@ -512,7 +506,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
> }
>
> #define RBD_MON_HOST 0
> -#define RBD_AUTH_SUPPORTED 1
>
> static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
> Error **errp)
> @@ -527,7 +520,7 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
> Error *local_err = NULL;
> int i;
>
> - assert(type == RBD_MON_HOST || type == RBD_AUTH_SUPPORTED);
> + assert(type == RBD_MON_HOST);
>
> num_entries = qdict_array_entries(options, prefix);
>
> @@ -573,10 +566,9 @@ static char *qemu_rbd_array_opts(QDict *options, const char *prefix, int type,
> value = strbuf;
> }
> } else {
> - value = qemu_opt_get(opts, "auth");
> + abort();
> }
>
> -
> /* each iteration in the for loop will build upon the string, and if
> * rados_str is NULL then it is our first pass */
> if (rados_str) {
> @@ -608,7 +600,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> QemuOpts *opts;
> Error *local_err = NULL;
> char *mon_host = NULL;
> - char *auth_supported = NULL;
> int r;
>
> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> @@ -619,14 +610,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> return -EINVAL;
> }
>
> - auth_supported = qemu_rbd_array_opts(options, "auth-supported.",
> - RBD_AUTH_SUPPORTED, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - r = -EINVAL;
> - goto failed_opts;
> - }
> -
> mon_host = qemu_rbd_array_opts(options, "server.",
> RBD_MON_HOST, &local_err);
> if (local_err) {
> @@ -678,13 +661,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> }
> }
>
> - if (auth_supported) {
> - r = rados_conf_set(s->cluster, "auth_supported", auth_supported);
> - if (r < 0) {
> - goto failed_shutdown;
> - }
> - }
> -
> if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
> r = -EIO;
> goto failed_shutdown;
> @@ -735,7 +711,6 @@ failed_shutdown:
> failed_opts:
> qemu_opts_del(opts);
> g_free(mon_host);
> - g_free(auth_supported);
> return r;
> }
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5d2efe4..6a7ca0b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2601,27 +2601,6 @@
>
>
> ##
> -# @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' } }
> -
> -##
> # @BlockdevOptionsRbd:
> #
> # @pool: Ceph pool name.
> @@ -2639,8 +2618,6 @@
> # @server: Monitor host address and port. This maps
> # to the "mon_host" Ceph option.
> #
> -# @auth-supported: Authentication supported.
> -#
> # @password-secret: The ID of a QCryptoSecret object providing
> # the password for the login.
> #
> @@ -2653,7 +2630,6 @@
> '*snapshot': 'str',
> '*user': 'str',
> '*server': ['InetSocketAddressBase'],
> - '*auth-supported': ['RbdAuthMethod'],
> '*password-secret': 'str' } }
>
> ##
> --
> 2.7.4
>
© 2016 - 2026 Red Hat, Inc.