Re: [Qemu-block] [PATCH 15/27] rbd: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to rbd, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  20 +++-
>  block/rbd.c  | 137 
> +--
>  2 files changed, 108 insertions(+), 49 deletions(-)

Reviewed-by: Max Reitz 

Some comments below.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5b4cd6bd12..370fcd9584 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3415,6 +3415,24 @@
>  '*refcount-bits':   'int' } }
>  
>  ##
> +# @BlockdevCreateOptionsRbd:
> +#
> +# Driver specific image creation options for rbd/Ceph.
> +#
> +# @location Where to store the new image file

Maybe this should mention that location.snapshot is not allowed?

(And that location.server is ignored.  But is that even intended?)

> +# @size Size of the virtual disk in bytes
> +# @password-secret  ID of secret providing the password
> +# @cluster_size RBD object size

s/_/-/

> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsRbd',
> +  'data': { 'location': 'BlockdevOptionsRbd',
> +'size': 'size',
> +'*password-secret': 'str',
> +'*cluster-size' :   'size' } }
> +
> +##
>  # @BlockdevCreateNotSupported:
>  #
>  # This is used for all drivers that don't support creating images.

[...]

> diff --git a/block/rbd.c b/block/rbd.c
> index a76a5e8755..c164f70167 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c

[...]

> @@ -432,24 +409,87 @@ static int qemu_rbd_create(const char *filename, 
> QemuOpts *opts, Error **errp)

[...]

> +static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error 
> **errp)
> +{
> +BlockdevCreateOptions *create_options;
> +BlockdevCreateOptionsRbd *rbd_opts;
> +BlockdevOptionsRbd *loc;
> +Error *local_err = NULL;
> +const char *keypairs;
> +QDict *options = NULL;
> +int ret = 0;
> +
> +create_options = g_new0(BlockdevCreateOptions, 1);
> +create_options->driver = BLOCKDEV_DRIVER_RBD;
> +rbd_opts = &create_options->u.rbd;
> +
> +rbd_opts->location = g_new0(BlockdevOptionsRbd, 1);
> +
> +rbd_opts->password_secret = (char *) qemu_opt_get(opts, 
> "password-secret");
> +
> +/* Read out options */
> +rbd_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> +  BDRV_SECTOR_SIZE);
> +rbd_opts->cluster_size = qemu_opt_get_size_del(opts,
> +   BLOCK_OPT_CLUSTER_SIZE, 
> 0);
> +rbd_opts->has_cluster_size = (rbd_opts->cluster_size != 0);
> +
> +options = qdict_new();
> +qemu_rbd_parse_filename(filename, options, &local_err);
> +if (local_err) {
> +ret = -EINVAL;
> +error_propagate(errp, local_err);
> +goto exit;
> +}
> +
> +/*
> + * Caution: while qdict_get_try_str() is fine, getting non-string
> + * types would require more care.  When @options come from -blockdev
> + * or blockdev_add, its members are typed according to the QAPI
> + * schema, but when they come from -drive, they're all QString.
> + */
> +loc = rbd_opts->location;
> +loc->pool = g_strdup(qdict_get_try_str(options, "pool"));
> +loc->conf = g_strdup(qdict_get_try_str(options, "conf"));
> +loc->has_conf = !!rbd_opts->location->conf;
> +loc->user = g_strdup(qdict_get_try_str(options, "user"));
> +loc->has_user = !!rbd_opts->location->user;

"!!loc->conf" and "!!loc->user" would be shorter and maybe a bit easier
to get.

Max

> +loc->image= g_strdup(qdict_get_try_str(options, "image"));
> +keypairs  = qdict_get_try_str(options, "=keyvalue-pairs");
> +
> +ret = qemu_rbd_do_create(create_options, keypairs, errp);
> +if (ret < 0) {
> +goto exit;
> +}
>  
>  exit:
>  QDECREF(options);
> +qapi_free_BlockdevCreateOptions(create_options);
>  return ret;
>  }



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 15/27] rbd: Support .bdrv_co_create

2018-02-08 Thread Kevin Wolf 
This adds the .bdrv_co_create driver callback to rbd, which enables
image creation over QMP.

Signed-off-by: Kevin Wolf 
---
 qapi/block-core.json |  20 +++-
 block/rbd.c  | 137 +--
 2 files changed, 108 insertions(+), 49 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b4cd6bd12..370fcd9584 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3415,6 +3415,24 @@
 '*refcount-bits':   'int' } }
 
 ##
+# @BlockdevCreateOptionsRbd:
+#
+# Driver specific image creation options for rbd/Ceph.
+#
+# @location Where to store the new image file
+# @size Size of the virtual disk in bytes
+# @password-secret  ID of secret providing the password
+# @cluster_size RBD object size
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsRbd',
+  'data': { 'location': 'BlockdevOptionsRbd',
+'size': 'size',
+'*password-secret': 'str',
+'*cluster-size' :   'size' } }
+
+##
 # @BlockdevCreateNotSupported:
 #
 # This is used for all drivers that don't support creating images.
@@ -3462,7 +3480,7 @@
   'qed':'BlockdevCreateNotSupported',
   'quorum': 'BlockdevCreateNotSupported',
   'raw':'BlockdevCreateNotSupported',
-  'rbd':'BlockdevCreateNotSupported',
+  'rbd':'BlockdevCreateOptionsRbd',
   'replication':'BlockdevCreateNotSupported',
   'sheepdog':   'BlockdevCreateNotSupported',
   'ssh':'BlockdevCreateNotSupported',
diff --git a/block/rbd.c b/block/rbd.c
index a76a5e8755..c164f70167 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -348,69 +348,46 @@ static QemuOptsList runtime_opts = {
 },
 };
 
-static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
+/* FIXME Deprecate and remove keypairs or make it available in QMP */
+static int qemu_rbd_do_create(BlockdevCreateOptions *options,
+  const char *keypairs, Error **errp)
 {
-Error *local_err = NULL;
-int64_t bytes = 0;
-int64_t objsize;
-int obj_order = 0;
-const char *pool, *image_name, *conf, *user, *keypairs;
-const char *secretid;
+BlockdevCreateOptionsRbd *opts = &options->u.rbd;
 rados_t cluster;
 rados_ioctx_t io_ctx;
-QDict *options = NULL;
-int ret = 0;
+int obj_order = 0;
+int ret;
 
-secretid = qemu_opt_get(opts, "password-secret");
+assert(options->driver == BLOCKDEV_DRIVER_RBD);
+if (opts->location->has_snapshot) {
+error_setg(errp, "Can't use snapshot name for image creation");
+return -EINVAL;
+}
 
-/* Read out options */
-bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
-objsize = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0);
-if (objsize) {
+if (opts->has_cluster_size) {
+int64_t objsize = opts->cluster_size;
 if ((objsize - 1) & objsize) {/* not a power of 2? */
 error_setg(errp, "obj size needs to be power of 2");
-ret = -EINVAL;
-goto exit;
+return -EINVAL;
 }
 if (objsize < 4096) {
 error_setg(errp, "obj size too small");
-ret = -EINVAL;
-goto exit;
+return -EINVAL;
 }
 obj_order = ctz32(objsize);
 }
 
-options = qdict_new();
-qemu_rbd_parse_filename(filename, options, &local_err);
-if (local_err) {
-ret = -EINVAL;
-error_propagate(errp, local_err);
-goto exit;
-}
-
-/*
- * Caution: while qdict_get_try_str() is fine, getting non-string
- * types would require more care.  When @options come from -blockdev
- * or blockdev_add, its members are typed according to the QAPI
- * schema, but when they come from -drive, they're all QString.
- */
-pool   = qdict_get_try_str(options, "pool");
-conf   = qdict_get_try_str(options, "conf");
-user   = qdict_get_try_str(options, "user");
-image_name = qdict_get_try_str(options, "image");
-keypairs   = qdict_get_try_str(options, "=keyvalue-pairs");
-
-ret = rados_create(&cluster, user);
+ret = rados_create(&cluster, opts->location->user);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "error initializing");
-goto exit;
+return ret;
 }
 
 /* try default location when conf=NULL, but ignore failure */
-ret = rados_conf_read_file(cluster, conf);
-if (conf && ret < 0) {
-error_setg_errno(errp, -ret, "error reading conf file %s", conf);
+ret = rados_conf_read_file(cluster, opts->location->conf);
+if (opts->location->conf && ret < 0) {
+error_setg_errno(errp, -ret, "error reading conf file %s",
+ opts->location->conf);
 ret = -EIO;
 goto shutdown;
 }