Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()
Am 23.02.2018 um 17:43 hat Max Reitz geschrieben: > On 2018-02-23 17:19, Kevin Wolf wrote: > > Am 23.02.2018 um 00:25 hat Max Reitz geschrieben: > >> 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 > > > >>> -*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? > > > > These days, both the QMP marshalling code (for the outermost struct when > > called from x-blockdev-create) and the input visitor (for nested structs > > and non-QMP callers) initialise the objects with {0} and g_malloc0(). > > > > I think Markus once told me that I shouldn't do pointless has_* checks > > any more in QMP commands, so I intentionally did the same here. > > I'm a bit cautious because of non-zero defaults (like sslverify in the > ssh driver), but as long as you're aware... I still hope that QAPI will allow specifying default values in the schema sometime. But yes, for the time being, not checking has_* obviously only works when the default is 0/false/NULL. Kevin signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()
On 2018-02-23 17:19, Kevin Wolf wrote: > Am 23.02.2018 um 00:25 hat Max Reitz geschrieben: >> 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 > >>> -*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? > > These days, both the QMP marshalling code (for the outermost struct when > called from x-blockdev-create) and the input visitor (for nested structs > and non-QMP callers) initialise the objects with {0} and g_malloc0(). > > I think Markus once told me that I shouldn't do pointless has_* checks > any more in QMP commands, so I intentionally did the same here. I'm a bit cautious because of non-zero defaults (like sslverify in the ssh driver), but as long as you're aware... Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()
Am 23.02.2018 um 00:25 hat Max Reitz geschrieben: > 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 > > -*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? These days, both the QMP marshalling code (for the outermost struct when called from x-blockdev-create) and the input visitor (for nested structs and non-QMP callers) initialise the objects with {0} and g_malloc0(). I think Markus once told me that I shouldn't do pointless has_* checks any more in QMP commands, so I intentionally did the same here. Kevin signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()
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 > --- > 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 a
[Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()
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 --- 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 @@ -24,6 +24,8 @@ #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qlist.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi-visit.h" /* * When specifying the image filename use: @@ -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; @@ -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) { +error_setg_errno(errp, -r, "error reading conf file %s", opts->conf); goto failed_shutdown; } @@ -613,13 +596,12 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, goto failed_shutdown; } -r = rados_ioctx_create(*cluster, pool, io_ctx); +r = rados_ioctx_create(*cluster, opts->pool, io_ctx); if (r < 0) { -error_setg_errno(errp, -r, "error opening pool %s", pool); +error_setg_errno(errp, -r, "error opening pool %s", opts->pool); goto failed_shutdown; } -qemu_opts_del(opts); return 0; failed_shutdown: @@ -627,7 +609,6 @@ failed_shutdown: g_free(*s_snap); g_free(*s_image_name); failed_opts: -qemu_opts_del(opts); g_free(mon_host); return r; } @@ -636,6 +617,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVRBDState *s = bs->opaque; +BlockdevOptionsRbd *opts = N