Re: [Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

[Kevin Wolf]

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()

[Max Reitz]

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()

[Kevin Wolf]

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()

[Max Reitz]

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(_opts, NULL, 0, _abort);
> -qemu_opts_absorb_qdict(opts, options, _err);
> +mon_host = qemu_rbd_mon_host(opts, _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
>  r = -EINVAL;
>  goto failed_opts;
>  }
>  
> -mon_host = qemu_rbd_mon_host(options, _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.

[Qemu-devel] [PATCH v2 21/36] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect()

[Kevin Wolf]

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(_opts, NULL, 0, _abort);
-qemu_opts_absorb_qdict(opts, options, _err);
+mon_host = qemu_rbd_mon_host(opts, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 r = -EINVAL;
 goto failed_opts;
 }
 
-mon_host = qemu_rbd_mon_host(options, _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 = NULL;
+