[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_r
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect() |
Date: |
Fri, 23 Feb 2018 00:25:18 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 2018-02-21 14:53, Kevin Wolf wrote:
> With the conversion to a QAPI options object, the function is now
> prepared to be used in a .bdrv_co_create implementation.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> block/rbd.c | 102
> +++++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 52 insertions(+), 50 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 2e79c2d1fd..26641e53e0 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
[...]
> @@ -482,24 +484,27 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
> qemu_aio_unref(acb);
> }
>
> -static char *qemu_rbd_mon_host(QDict *options, Error **errp)
> +static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp)
> {
> - const char **vals = g_new(const char *, qdict_size(options) + 1);
> - char keybuf[32];
> + const char **vals;
> const char *host, *port;
> char *rados_str;
> - int i;
> -
> - for (i = 0;; i++) {
> - sprintf(keybuf, "server.%d.host", i);
> - host = qdict_get_try_str(options, keybuf);
> - qdict_del(options, keybuf);
> - sprintf(keybuf, "server.%d.port", i);
> - port = qdict_get_try_str(options, keybuf);
> - qdict_del(options, keybuf);
> - if (!host && !port) {
> - break;
> - }
> + InetSocketAddressBaseList *p;
> + int i, cnt;
> +
> + if (!opts->has_server) {
> + return NULL;
> + }
> +
> + for (cnt = 0, p = opts->server; p; p = p->next) {
> + cnt++;
> + }
> +
> + vals = g_new(const char *, cnt + 1);
> +
> + for (i = 0, p = opts->server; p; p = p->next, i++) {
> + host = p->value->host;
> + port = p->value->port;
> if (!host) {
> error_setg(errp, "Parameter server.%d.host is missing", i);
> rados_str = NULL;
host and port are mandatory in the schema, so we can omit this now.
> @@ -524,56 +529,34 @@ out:
>
> static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx,
> char **s_snap, char **s_image_name,
> - QDict *options, bool cache,
> + BlockdevOptionsRbd *opts, bool cache,
> const char *keypairs, const char *secretid,
> Error **errp)
> {
> - QemuOpts *opts;
> char *mon_host = NULL;
> - const char *pool, *snap, *conf, *user, *image_name;
> Error *local_err = NULL;
> int r;
>
> - opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> - qemu_opts_absorb_qdict(opts, options, &local_err);
> + mon_host = qemu_rbd_mon_host(opts, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> r = -EINVAL;
> goto failed_opts;
> }
>
> - mon_host = qemu_rbd_mon_host(options, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - r = -EINVAL;
> - goto failed_opts;
> - }
> -
> - pool = qemu_opt_get(opts, "pool");
> - conf = qemu_opt_get(opts, "conf");
> - snap = qemu_opt_get(opts, "snapshot");
> - user = qemu_opt_get(opts, "user");
> - image_name = qemu_opt_get(opts, "image");
> -
> - if (!pool || !image_name) {
> - error_setg(errp, "Parameters 'pool' and 'image' are required");
> - r = -EINVAL;
> - goto failed_opts;
> - }
> -
> - r = rados_create(cluster, user);
> + r = rados_create(cluster, opts->user);
> if (r < 0) {
> error_setg_errno(errp, -r, "error initializing");
> goto failed_opts;
> }
>
> - *s_snap = g_strdup(snap);
> - *s_image_name = g_strdup(image_name);
> + *s_snap = g_strdup(opts->snapshot);
> + *s_image_name = g_strdup(opts->image);
>
> /* try default location when conf=NULL, but ignore failure */
> - r = rados_conf_read_file(*cluster, conf);
> - if (conf && r < 0) {
> - error_setg_errno(errp, -r, "error reading conf file %s", conf);
> + r = rados_conf_read_file(*cluster, opts->conf);
> + if (opts->has_conf && r < 0) {
Reading opts->conf without knowing whether opts->has_conf is true is a
bit weird. Would you mind "s->has_conf ? opts->conf : NULL" for the
rados_conf_read() call?
On that thought, opts->snapshot and opts->user are optional, too. Are
they guaranteed to be NULL if they haven't been specified? Should we
guard those accesses with opts->has_* queries, too?
> + error_setg_errno(errp, -r, "error reading conf file %s", opts->conf);
> goto failed_shutdown;
> }
>
[..]
> @@ -666,8 +650,25 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> *options, int flags,
> qdict_del(options, "password-secret");
> }
>
> + /* Convert the remaining options into a QAPI object */
> + crumpled = qdict_crumple(options, errp);
> + if (crumpled == NULL) {
> + return -EINVAL;
"goto out", or this will leak @keypairs and @secretid.
Max
> + }
> +
> + v = qobject_input_visitor_new_keyval(crumpled);
> + visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
> + visit_free(v);
> + qobject_decref(crumpled);
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + r = -EINVAL;
> + goto out;
> + }
> +
> r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name,
> - options, !(flags & BDRV_O_NOCACHE), keypairs,
> secretid,
> + opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid,
> errp);
> if (r < 0) {
> goto out;
> @@ -706,6 +707,7 @@ failed_open:
> g_free(s->image_name);
> rados_shutdown(s->cluster);
> out:
> + qapi_free_BlockdevOptionsRbd(opts);
> g_free(keypairs);
> g_free(secretid);
> return r;
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 19/36] rbd: Factor out qemu_rbd_connect(), (continued)
- [Qemu-devel] [PATCH v2 19/36] rbd: Factor out qemu_rbd_connect(), Kevin Wolf, 2018/02/21
- [Qemu-devel] [PATCH v2 16/36] file-win32: Support .bdrv_co_create, Kevin Wolf, 2018/02/21
- [Qemu-devel] [PATCH v2 24/36] rbd: Use qemu_rbd_connect() in qemu_rbd_do_create(), Kevin Wolf, 2018/02/21
- [Qemu-devel] [PATCH v2 23/36] rbd: Assing s->snap/image_name in qemu_rbd_open(), Kevin Wolf, 2018/02/21
- [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect(), Kevin Wolf, 2018/02/21
- Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect(),
Max Reitz <=
[Qemu-devel] [PATCH v2 25/36] nfs: Use QAPI options in nfs_client_open(), Kevin Wolf, 2018/02/21
[Qemu-devel] [PATCH v2 22/36] rbd: Support .bdrv_co_create, Kevin Wolf, 2018/02/21
[Qemu-devel] [PATCH v2 26/36] nfs: Support .bdrv_co_create, Kevin Wolf, 2018/02/21
[Qemu-devel] [PATCH v2 27/36] sheepdog: QAPIfy "redundacy" create option, Kevin Wolf, 2018/02/21