* [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-12 16:40 ` Max Reitz
2018-03-12 21:30 ` Jeff Cody
2018-03-09 21:46 ` [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels Kevin Wolf
` (5 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
This adds the .bdrv_co_create driver callback to parallels, which
enables image creation over QMP.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 18 ++++-
block/parallels.c | 199 ++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 168 insertions(+), 49 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 07039bfe9c..d38058eeab 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3481,6 +3481,22 @@
'size': 'size' } }
##
+# @BlockdevCreateOptionsParallels:
+#
+# Driver specific image creation options for parallels.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @cluster-size Cluster size in bytes (default: 1 MB)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsParallels',
+ 'data': { 'file': 'BlockdevRef',
+ 'size': 'size',
+ '*cluster-size': 'size' } }
+
+##
# @BlockdevQcow2Version:
#
# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
@@ -3664,7 +3680,7 @@
'null-aio': 'BlockdevCreateNotSupported',
'null-co': 'BlockdevCreateNotSupported',
'nvme': 'BlockdevCreateNotSupported',
- 'parallels': 'BlockdevCreateNotSupported',
+ 'parallels': 'BlockdevCreateOptionsParallels',
'qcow2': 'BlockdevCreateOptionsQcow2',
'qcow': 'BlockdevCreateNotSupported',
'qed': 'BlockdevCreateNotSupported',
diff --git a/block/parallels.c b/block/parallels.c
index c13cb619e6..2da5e56a9d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -34,6 +34,9 @@
#include "sysemu/block-backend.h"
#include "qemu/module.h"
#include "qemu/option.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-block-core.h"
#include "qemu/bswap.h"
#include "qemu/bitmap.h"
#include "migration/blocker.h"
@@ -79,6 +82,25 @@ static QemuOptsList parallels_runtime_opts = {
},
};
+static QemuOptsList parallels_create_opts = {
+ .name = "parallels-create-opts",
+ .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head),
+ .desc = {
+ {
+ .name = BLOCK_OPT_SIZE,
+ .type = QEMU_OPT_SIZE,
+ .help = "Virtual disk size",
+ },
+ {
+ .name = BLOCK_OPT_CLUSTER_SIZE,
+ .type = QEMU_OPT_SIZE,
+ .help = "Parallels image cluster size",
+ .def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
+ },
+ { /* end of list */ }
+ }
+};
+
static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
{
@@ -480,46 +502,62 @@ out:
}
-static int coroutine_fn parallels_co_create_opts(const char *filename,
- QemuOpts *opts,
- Error **errp)
+static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
+ Error **errp)
{
+ BlockdevCreateOptionsParallels *parallels_opts;
+ BlockDriverState *bs;
+ BlockBackend *blk;
int64_t total_size, cl_size;
- uint8_t tmp[BDRV_SECTOR_SIZE];
- Error *local_err = NULL;
- BlockBackend *file;
uint32_t bat_entries, bat_sectors;
ParallelsHeader header;
+ uint8_t tmp[BDRV_SECTOR_SIZE];
int ret;
- total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
- cl_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
- DEFAULT_CLUSTER_SIZE), BDRV_SECTOR_SIZE);
+ assert(opts->driver == BLOCKDEV_DRIVER_PARALLELS);
+ parallels_opts = &opts->u.parallels;
+
+ /* Sanity checks */
+ total_size = parallels_opts->size;
+
+ if (parallels_opts->has_cluster_size) {
+ cl_size = parallels_opts->cluster_size;
+ } else {
+ cl_size = DEFAULT_CLUSTER_SIZE;
+ }
+
if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) {
- error_propagate(errp, local_err);
+ error_setg(errp, "Image size is too large for this cluster size");
return -E2BIG;
}
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- return ret;
+ if (!QEMU_IS_ALIGNED(total_size, BDRV_SECTOR_SIZE)) {
+ error_setg(errp, "Image size must be a multiple of 512 bytes");
+ return -EINVAL;
}
- file = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (file == NULL) {
- error_propagate(errp, local_err);
+ if (!QEMU_IS_ALIGNED(cl_size, BDRV_SECTOR_SIZE)) {
+ error_setg(errp, "Cluster size must be a multiple of 512 bytes");
+ return -EINVAL;
+ }
+
+ /* Create BlockBackend to write to the image */
+ bs = bdrv_open_blockdev_ref(parallels_opts->file, errp);
+ if (bs == NULL) {
return -EIO;
}
- blk_set_allow_write_beyond_eof(file, true);
+ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ ret = blk_insert_bs(blk, bs, errp);
+ if (ret < 0) {
+ goto out;
+ }
+ blk_set_allow_write_beyond_eof(blk, true);
- ret = blk_truncate(file, 0, PREALLOC_MODE_OFF, errp);
+ /* Create image format */
+ ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
if (ret < 0) {
- goto exit;
+ goto out;
}
bat_entries = DIV_ROUND_UP(total_size, cl_size);
@@ -542,24 +580,107 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
memset(tmp, 0, sizeof(tmp));
memcpy(tmp, &header, sizeof(header));
- ret = blk_pwrite(file, 0, tmp, BDRV_SECTOR_SIZE, 0);
+ ret = blk_pwrite(blk, 0, tmp, BDRV_SECTOR_SIZE, 0);
if (ret < 0) {
goto exit;
}
- ret = blk_pwrite_zeroes(file, BDRV_SECTOR_SIZE,
+ ret = blk_pwrite_zeroes(blk, BDRV_SECTOR_SIZE,
(bat_sectors - 1) << BDRV_SECTOR_BITS, 0);
if (ret < 0) {
goto exit;
}
- ret = 0;
-done:
- blk_unref(file);
+ ret = 0;
+out:
+ blk_unref(blk);
+ bdrv_unref(bs);
return ret;
exit:
error_setg_errno(errp, -ret, "Failed to create Parallels image");
- goto done;
+ goto out;
+}
+
+static int coroutine_fn parallels_co_create_opts(const char *filename,
+ QemuOpts *opts,
+ Error **errp)
+{
+ BlockdevCreateOptions *create_options = NULL;
+ Error *local_err = NULL;
+ BlockDriverState *bs = NULL;
+ QDict *qdict = NULL;
+ QObject *qobj;
+ Visitor *v;
+ int ret;
+
+ static const QDictRenames opt_renames[] = {
+ { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" },
+ { NULL, NULL },
+ };
+
+ /* Parse options and convert legacy syntax */
+ qdict = qemu_opts_to_qdict_filtered(opts, NULL, ¶llels_create_opts,
+ true);
+
+ if (!qdict_rename_keys(qdict, opt_renames, errp)) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ /* Create and open the file (protocol layer) */
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto done;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto done;
+ }
+
+ /* Now get the QAPI type BlockdevCreateOptions */
+ qdict_put_str(qdict, "driver", "parallels");
+ qdict_put_str(qdict, "file", bs->node_name);
+
+ qobj = qdict_crumple(qdict, errp);
+ QDECREF(qdict);
+ qdict = qobject_to_qdict(qobj);
+ if (qdict == NULL) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+ visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+ visit_free(v);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto done;
+ }
+
+ /* Silently round up sizes */
+ create_options->u.parallels.size =
+ ROUND_UP(create_options->u.parallels.size, BDRV_SECTOR_SIZE);
+ create_options->u.parallels.cluster_size =
+ ROUND_UP(create_options->u.parallels.cluster_size, BDRV_SECTOR_SIZE);
+
+ /* Create the Parallels image (format layer) */
+ ret = parallels_co_create(create_options, errp);
+ if (ret < 0) {
+ goto done;
+ }
+ ret = 0;
+
+done:
+ QDECREF(qdict);
+ bdrv_unref(bs);
+ qapi_free_BlockdevCreateOptions(create_options);
+ return ret;
}
@@ -771,25 +892,6 @@ static void parallels_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
-static QemuOptsList parallels_create_opts = {
- .name = "parallels-create-opts",
- .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head),
- .desc = {
- {
- .name = BLOCK_OPT_SIZE,
- .type = QEMU_OPT_SIZE,
- .help = "Virtual disk size",
- },
- {
- .name = BLOCK_OPT_CLUSTER_SIZE,
- .type = QEMU_OPT_SIZE,
- .help = "Parallels image cluster size",
- .def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
- },
- { /* end of list */ }
- }
-};
-
static BlockDriver bdrv_parallels = {
.format_name = "parallels",
.instance_size = sizeof(BDRVParallelsState),
@@ -803,6 +905,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_readv = parallels_co_readv,
.bdrv_co_writev = parallels_co_writev,
.supports_backing = true,
+ .bdrv_co_create = parallels_co_create,
.bdrv_co_create_opts = parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
.create_opts = ¶llels_create_opts,
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create Kevin Wolf
@ 2018-03-12 16:40 ` Max Reitz
2018-03-12 21:30 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-12 16:40 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to parallels, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 18 ++++-
> block/parallels.c | 199 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 168 insertions(+), 49 deletions(-)
[...]
> diff --git a/block/parallels.c b/block/parallels.c
> index c13cb619e6..2da5e56a9d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
[...]
> @@ -542,24 +580,107 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
[...]
> +static int coroutine_fn parallels_co_create_opts(const char *filename,
> + QemuOpts *opts,
> + Error **errp)
> +{
[...]
> + /* Now get the QAPI type BlockdevCreateOptions */
> + qdict_put_str(qdict, "driver", "parallels");
> + qdict_put_str(qdict, "file", bs->node_name);
> +
> + qobj = qdict_crumple(qdict, errp);
Any reason for this crumpling other than because it doesn't change
anything and if it did, it would be for the better?
The rest looks OK to me, and I just think this creates an exact copy of
qdict, so I guess:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> + QDECREF(qdict);
> + qdict = qobject_to_qdict(qobj);
> + if (qdict == NULL) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
> + visit_free(v);
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create Kevin Wolf
2018-03-12 16:40 ` Max Reitz
@ 2018-03-12 21:30 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2018-03-12 21:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, den, eblake, qemu-devel
On Fri, Mar 09, 2018 at 10:46:05PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to parallels, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 18 ++++-
> block/parallels.c | 199 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 168 insertions(+), 49 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 07039bfe9c..d38058eeab 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3481,6 +3481,22 @@
> 'size': 'size' } }
>
> ##
> +# @BlockdevCreateOptionsParallels:
> +#
> +# Driver specific image creation options for parallels.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @cluster-size Cluster size in bytes (default: 1 MB)
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsParallels',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*cluster-size': 'size' } }
> +
> +##
> # @BlockdevQcow2Version:
> #
> # @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
> @@ -3664,7 +3680,7 @@
> 'null-aio': 'BlockdevCreateNotSupported',
> 'null-co': 'BlockdevCreateNotSupported',
> 'nvme': 'BlockdevCreateNotSupported',
> - 'parallels': 'BlockdevCreateNotSupported',
> + 'parallels': 'BlockdevCreateOptionsParallels',
> 'qcow2': 'BlockdevCreateOptionsQcow2',
> 'qcow': 'BlockdevCreateNotSupported',
> 'qed': 'BlockdevCreateNotSupported',
> diff --git a/block/parallels.c b/block/parallels.c
> index c13cb619e6..2da5e56a9d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -34,6 +34,9 @@
> #include "sysemu/block-backend.h"
> #include "qemu/module.h"
> #include "qemu/option.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qapi-visit-block-core.h"
> #include "qemu/bswap.h"
> #include "qemu/bitmap.h"
> #include "migration/blocker.h"
> @@ -79,6 +82,25 @@ static QemuOptsList parallels_runtime_opts = {
> },
> };
>
> +static QemuOptsList parallels_create_opts = {
> + .name = "parallels-create-opts",
> + .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head),
> + .desc = {
> + {
> + .name = BLOCK_OPT_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Virtual disk size",
> + },
> + {
> + .name = BLOCK_OPT_CLUSTER_SIZE,
> + .type = QEMU_OPT_SIZE,
> + .help = "Parallels image cluster size",
> + .def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
> + },
> + { /* end of list */ }
> + }
> +};
> +
>
> static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
> {
> @@ -480,46 +502,62 @@ out:
> }
>
>
> -static int coroutine_fn parallels_co_create_opts(const char *filename,
> - QemuOpts *opts,
> - Error **errp)
> +static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
> + Error **errp)
> {
> + BlockdevCreateOptionsParallels *parallels_opts;
> + BlockDriverState *bs;
> + BlockBackend *blk;
> int64_t total_size, cl_size;
> - uint8_t tmp[BDRV_SECTOR_SIZE];
> - Error *local_err = NULL;
> - BlockBackend *file;
> uint32_t bat_entries, bat_sectors;
> ParallelsHeader header;
> + uint8_t tmp[BDRV_SECTOR_SIZE];
> int ret;
>
> - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> - BDRV_SECTOR_SIZE);
> - cl_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> - DEFAULT_CLUSTER_SIZE), BDRV_SECTOR_SIZE);
> + assert(opts->driver == BLOCKDEV_DRIVER_PARALLELS);
> + parallels_opts = &opts->u.parallels;
> +
> + /* Sanity checks */
> + total_size = parallels_opts->size;
> +
> + if (parallels_opts->has_cluster_size) {
> + cl_size = parallels_opts->cluster_size;
> + } else {
> + cl_size = DEFAULT_CLUSTER_SIZE;
> + }
> +
> if (total_size >= MAX_PARALLELS_IMAGE_FACTOR * cl_size) {
> - error_propagate(errp, local_err);
> + error_setg(errp, "Image size is too large for this cluster size");
> return -E2BIG;
> }
>
> - ret = bdrv_create_file(filename, opts, &local_err);
> - if (ret < 0) {
> - error_propagate(errp, local_err);
> - return ret;
> + if (!QEMU_IS_ALIGNED(total_size, BDRV_SECTOR_SIZE)) {
> + error_setg(errp, "Image size must be a multiple of 512 bytes");
> + return -EINVAL;
> }
>
> - file = blk_new_open(filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> - &local_err);
> - if (file == NULL) {
> - error_propagate(errp, local_err);
> + if (!QEMU_IS_ALIGNED(cl_size, BDRV_SECTOR_SIZE)) {
> + error_setg(errp, "Cluster size must be a multiple of 512 bytes");
> + return -EINVAL;
> + }
> +
> + /* Create BlockBackend to write to the image */
> + bs = bdrv_open_blockdev_ref(parallels_opts->file, errp);
> + if (bs == NULL) {
> return -EIO;
> }
>
> - blk_set_allow_write_beyond_eof(file, true);
> + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
> + ret = blk_insert_bs(blk, bs, errp);
> + if (ret < 0) {
> + goto out;
> + }
> + blk_set_allow_write_beyond_eof(blk, true);
>
> - ret = blk_truncate(file, 0, PREALLOC_MODE_OFF, errp);
> + /* Create image format */
> + ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
> if (ret < 0) {
> - goto exit;
> + goto out;
> }
>
> bat_entries = DIV_ROUND_UP(total_size, cl_size);
> @@ -542,24 +580,107 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
> memset(tmp, 0, sizeof(tmp));
> memcpy(tmp, &header, sizeof(header));
>
> - ret = blk_pwrite(file, 0, tmp, BDRV_SECTOR_SIZE, 0);
> + ret = blk_pwrite(blk, 0, tmp, BDRV_SECTOR_SIZE, 0);
> if (ret < 0) {
> goto exit;
> }
> - ret = blk_pwrite_zeroes(file, BDRV_SECTOR_SIZE,
> + ret = blk_pwrite_zeroes(blk, BDRV_SECTOR_SIZE,
> (bat_sectors - 1) << BDRV_SECTOR_BITS, 0);
> if (ret < 0) {
> goto exit;
> }
> - ret = 0;
>
> -done:
> - blk_unref(file);
> + ret = 0;
> +out:
> + blk_unref(blk);
> + bdrv_unref(bs);
> return ret;
>
> exit:
> error_setg_errno(errp, -ret, "Failed to create Parallels image");
> - goto done;
> + goto out;
> +}
> +
> +static int coroutine_fn parallels_co_create_opts(const char *filename,
> + QemuOpts *opts,
> + Error **errp)
> +{
> + BlockdevCreateOptions *create_options = NULL;
> + Error *local_err = NULL;
> + BlockDriverState *bs = NULL;
> + QDict *qdict = NULL;
> + QObject *qobj;
> + Visitor *v;
> + int ret;
> +
> + static const QDictRenames opt_renames[] = {
> + { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" },
> + { NULL, NULL },
> + };
> +
> + /* Parse options and convert legacy syntax */
> + qdict = qemu_opts_to_qdict_filtered(opts, NULL, ¶llels_create_opts,
> + true);
> +
> + if (!qdict_rename_keys(qdict, opt_renames, errp)) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + /* Create and open the file (protocol layer) */
> + ret = bdrv_create_file(filename, opts, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto done;
> + }
> +
> + bs = bdrv_open(filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
> + if (bs == NULL) {
> + ret = -EIO;
> + goto done;
> + }
> +
> + /* Now get the QAPI type BlockdevCreateOptions */
> + qdict_put_str(qdict, "driver", "parallels");
> + qdict_put_str(qdict, "file", bs->node_name);
> +
> + qobj = qdict_crumple(qdict, errp);
> + QDECREF(qdict);
> + qdict = qobject_to_qdict(qobj);
> + if (qdict == NULL) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
> + visit_free(v);
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + /* Silently round up sizes */
> + create_options->u.parallels.size =
> + ROUND_UP(create_options->u.parallels.size, BDRV_SECTOR_SIZE);
> + create_options->u.parallels.cluster_size =
> + ROUND_UP(create_options->u.parallels.cluster_size, BDRV_SECTOR_SIZE);
> +
> + /* Create the Parallels image (format layer) */
> + ret = parallels_co_create(create_options, errp);
> + if (ret < 0) {
> + goto done;
> + }
> + ret = 0;
> +
> +done:
> + QDECREF(qdict);
> + bdrv_unref(bs);
> + qapi_free_BlockdevCreateOptions(create_options);
> + return ret;
> }
>
>
> @@ -771,25 +892,6 @@ static void parallels_close(BlockDriverState *bs)
> error_free(s->migration_blocker);
> }
>
> -static QemuOptsList parallels_create_opts = {
> - .name = "parallels-create-opts",
> - .head = QTAILQ_HEAD_INITIALIZER(parallels_create_opts.head),
> - .desc = {
> - {
> - .name = BLOCK_OPT_SIZE,
> - .type = QEMU_OPT_SIZE,
> - .help = "Virtual disk size",
> - },
> - {
> - .name = BLOCK_OPT_CLUSTER_SIZE,
> - .type = QEMU_OPT_SIZE,
> - .help = "Parallels image cluster size",
> - .def_value_str = stringify(DEFAULT_CLUSTER_SIZE),
> - },
> - { /* end of list */ }
> - }
> -};
> -
> static BlockDriver bdrv_parallels = {
> .format_name = "parallels",
> .instance_size = sizeof(BDRVParallelsState),
> @@ -803,6 +905,7 @@ static BlockDriver bdrv_parallels = {
> .bdrv_co_readv = parallels_co_readv,
> .bdrv_co_writev = parallels_co_writev,
> .supports_backing = true,
> + .bdrv_co_create = parallels_co_create,
> .bdrv_co_create_opts = parallels_co_create_opts,
> .bdrv_co_check = parallels_co_check,
> .create_opts = ¶llels_create_opts,
> --
> 2.13.6
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
2018-03-09 21:46 ` [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-12 16:42 ` Max Reitz
2018-03-12 21:31 ` Jeff Cody
2018-03-09 21:46 ` [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create Kevin Wolf
` (4 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
Originally we added parallels as a read-only format to qemu-iotests
where we did just some tests with a binary image. Since then, write and
image creation support has been added to the driver, so we can now
enable it in _supported_fmt generic.
The driver doesn't support migration yet, though, so we need to add it
to the list of exceptions in 181.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/181 | 2 +-
tests/qemu-iotests/check | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/181 b/tests/qemu-iotests/181
index 0c91e8f9de..5e767c6195 100755
--- a/tests/qemu-iotests/181
+++ b/tests/qemu-iotests/181
@@ -44,7 +44,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt generic
# Formats that do not support live migration
-_unsupported_fmt qcow vdi vhdx vmdk vpc vvfat
+_unsupported_fmt qcow vdi vhdx vmdk vpc vvfat parallels
_supported_proto generic
_supported_os Linux
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e6b6ff7a04..469142cd58 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -284,7 +284,6 @@ testlist options
-parallels)
IMGFMT=parallels
- IMGFMT_GENERIC=false
xpand=false
;;
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels
2018-03-09 21:46 ` [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels Kevin Wolf
@ 2018-03-12 16:42 ` Max Reitz
2018-03-12 21:31 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-12 16:42 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 641 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> Originally we added parallels as a read-only format to qemu-iotests
> where we did just some tests with a binary image. Since then, write and
> image creation support has been added to the driver, so we can now
> enable it in _supported_fmt generic.
>
> The driver doesn't support migration yet, though, so we need to add it
> to the list of exceptions in 181.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/181 | 2 +-
> tests/qemu-iotests/check | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels
2018-03-09 21:46 ` [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels Kevin Wolf
2018-03-12 16:42 ` Max Reitz
@ 2018-03-12 21:31 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2018-03-12 21:31 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, den, eblake, qemu-devel
On Fri, Mar 09, 2018 at 10:46:06PM +0100, Kevin Wolf wrote:
> Originally we added parallels as a read-only format to qemu-iotests
> where we did just some tests with a binary image. Since then, write and
> image creation support has been added to the driver, so we can now
> enable it in _supported_fmt generic.
>
> The driver doesn't support migration yet, though, so we need to add it
> to the list of exceptions in 181.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/181 | 2 +-
> tests/qemu-iotests/check | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/181 b/tests/qemu-iotests/181
> index 0c91e8f9de..5e767c6195 100755
> --- a/tests/qemu-iotests/181
> +++ b/tests/qemu-iotests/181
> @@ -44,7 +44,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>
> _supported_fmt generic
> # Formats that do not support live migration
> -_unsupported_fmt qcow vdi vhdx vmdk vpc vvfat
> +_unsupported_fmt qcow vdi vhdx vmdk vpc vvfat parallels
> _supported_proto generic
> _supported_os Linux
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index e6b6ff7a04..469142cd58 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -284,7 +284,6 @@ testlist options
>
> -parallels)
> IMGFMT=parallels
> - IMGFMT_GENERIC=false
> xpand=false
> ;;
>
> --
> 2.13.6
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
2018-03-09 21:46 ` [Qemu-devel] [PATCH 1/7] parallels: Support .bdrv_co_create Kevin Wolf
2018-03-09 21:46 ` [Qemu-devel] [PATCH 2/7] qemu-iotests: Enable write tests for parallels Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-09 21:58 ` Eric Blake
` (3 more replies)
2018-03-09 21:46 ` [Qemu-devel] [PATCH 4/7] qed: " Kevin Wolf
` (3 subsequent siblings)
6 siblings, 4 replies; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
This adds the .bdrv_co_create driver callback to qcow, which
enables image creation over QMP.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 21 +++++-
block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
2 files changed, 150 insertions(+), 67 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d38058eeab..c81677c434 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3497,6 +3497,25 @@
'*cluster-size': 'size' } }
##
+# @BlockdevCreateOptionsQcow:
+#
+# Driver specific image creation options for qcow.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @backing-file File name of the backing file if a backing file
+# should be used
+# @encrypt Encryption options if the image should be encrypted
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsQcow',
+ 'data': { 'file': 'BlockdevRef',
+ 'size': 'size',
+ '*backing-file': 'str',
+ '*encrypt': 'QCryptoBlockCreateOptions' } }
+
+##
# @BlockdevQcow2Version:
#
# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
@@ -3681,8 +3700,8 @@
'null-co': 'BlockdevCreateNotSupported',
'nvme': 'BlockdevCreateNotSupported',
'parallels': 'BlockdevCreateOptionsParallels',
+ 'qcow': 'BlockdevCreateOptionsQcow',
'qcow2': 'BlockdevCreateOptionsQcow2',
- 'qcow': 'BlockdevCreateNotSupported',
'qed': 'BlockdevCreateNotSupported',
'quorum': 'BlockdevCreateNotSupported',
'raw': 'BlockdevCreateNotSupported',
diff --git a/block/qcow.c b/block/qcow.c
index 47a18d9a3a..2e3770ca63 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -33,6 +33,8 @@
#include <zlib.h>
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-block-core.h"
#include "crypto/block.h"
#include "migration/blocker.h"
#include "block/crypto.h"
@@ -86,6 +88,8 @@ typedef struct BDRVQcowState {
Error *migration_blocker;
} BDRVQcowState;
+static QemuOptsList qcow_create_opts;
+
static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -810,62 +814,50 @@ static void qcow_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
-static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
+static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
+ Error **errp)
{
+ BlockdevCreateOptionsQcow *qcow_opts;
int header_size, backing_filename_len, l1_size, shift, i;
QCowHeader header;
uint8_t *tmp;
int64_t total_size = 0;
- char *backing_file = NULL;
- Error *local_err = NULL;
int ret;
+ BlockDriverState *bs;
BlockBackend *qcow_blk;
- char *encryptfmt = NULL;
- QDict *options;
- QDict *encryptopts = NULL;
- QCryptoBlockCreateOptions *crypto_opts = NULL;
QCryptoBlock *crypto = NULL;
- /* Read out options */
- total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
+ assert(opts->driver == BLOCKDEV_DRIVER_QCOW);
+ qcow_opts = &opts->u.qcow;
+
+ /* Sanity checks */
+ total_size = qcow_opts->size;
if (total_size == 0) {
error_setg(errp, "Image size is too small, cannot be zero length");
- ret = -EINVAL;
- goto cleanup;
+ return -EINVAL;
}
- backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
- encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
- if (encryptfmt) {
- if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
- error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
- BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
- ret = -EINVAL;
- goto cleanup;
- }
- } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
- encryptfmt = g_strdup("aes");
+ if (qcow_opts->has_encrypt &&
+ qcow_opts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_QCOW)
+ {
+ error_setg(errp, "Unsupported encryption format");
+ return -EINVAL;
}
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- goto cleanup;
+ /* Create BlockBackend to write to the image */
+ bs = bdrv_open_blockdev_ref(qcow_opts->file, errp);
+ if (bs == NULL) {
+ return -EIO;
}
- qcow_blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (qcow_blk == NULL) {
- error_propagate(errp, local_err);
- ret = -EIO;
- goto cleanup;
+ qcow_blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ ret = blk_insert_bs(qcow_blk, bs, errp);
+ if (ret < 0) {
+ goto exit;
}
-
blk_set_allow_write_beyond_eof(qcow_blk, true);
+ /* Create image format */
ret = blk_truncate(qcow_blk, 0, PREALLOC_MODE_OFF, errp);
if (ret < 0) {
goto exit;
@@ -877,16 +869,15 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
header.size = cpu_to_be64(total_size);
header_size = sizeof(header);
backing_filename_len = 0;
- if (backing_file) {
- if (strcmp(backing_file, "fat:")) {
+ if (qcow_opts->has_backing_file) {
+ if (strcmp(qcow_opts->backing_file, "fat:")) {
header.backing_file_offset = cpu_to_be64(header_size);
- backing_filename_len = strlen(backing_file);
+ backing_filename_len = strlen(qcow_opts->backing_file);
header.backing_file_size = cpu_to_be32(backing_filename_len);
header_size += backing_filename_len;
} else {
/* special backing file for vvfat */
- g_free(backing_file);
- backing_file = NULL;
+ qcow_opts->has_backing_file = false;
}
header.cluster_bits = 9; /* 512 byte cluster to avoid copying
unmodified sectors */
@@ -901,26 +892,10 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
header.l1_table_offset = cpu_to_be64(header_size);
- options = qemu_opts_to_qdict(opts, NULL);
- qdict_extract_subqdict(options, &encryptopts, "encrypt.");
- QDECREF(options);
- if (encryptfmt) {
- if (!g_str_equal(encryptfmt, "aes")) {
- error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
- encryptfmt);
- ret = -EINVAL;
- goto exit;
- }
+ if (qcow_opts->has_encrypt) {
header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
- crypto_opts = block_crypto_create_opts_init(
- Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
- if (!crypto_opts) {
- ret = -EINVAL;
- goto exit;
- }
-
- crypto = qcrypto_block_create(crypto_opts, "encrypt.",
+ crypto = qcrypto_block_create(qcow_opts->encrypt, "encrypt.",
NULL, NULL, NULL, errp);
if (!crypto) {
ret = -EINVAL;
@@ -936,9 +911,9 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
goto exit;
}
- if (backing_file) {
+ if (qcow_opts->has_backing_file) {
ret = blk_pwrite(qcow_blk, sizeof(header),
- backing_file, backing_filename_len, 0);
+ qcow_opts->backing_file, backing_filename_len, 0);
if (ret != backing_filename_len) {
goto exit;
}
@@ -959,12 +934,100 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
ret = 0;
exit:
blk_unref(qcow_blk);
-cleanup:
- QDECREF(encryptopts);
- g_free(encryptfmt);
qcrypto_block_free(crypto);
- qapi_free_QCryptoBlockCreateOptions(crypto_opts);
- g_free(backing_file);
+ return ret;
+}
+
+static int coroutine_fn qcow_co_create_opts(const char *filename,
+ QemuOpts *opts, Error **errp)
+{
+ BlockdevCreateOptions *create_options = NULL;
+ BlockDriverState *bs = NULL;
+ QDict *qdict = NULL;
+ QObject *qobj;
+ Visitor *v;
+ const char *val;
+ Error *local_err = NULL;
+ int ret;
+
+ static const QDictRenames opt_renames[] = {
+ { BLOCK_OPT_BACKING_FILE, "backing-file" },
+ { BLOCK_OPT_ENCRYPT, BLOCK_OPT_ENCRYPT_FORMAT },
+ { NULL, NULL },
+ };
+
+ /* Parse options and convert legacy syntax */
+ qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qcow_create_opts, true);
+
+ val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
+ if (val && !strcmp(val, "on")) {
+ qdict_put_str(qdict, BLOCK_OPT_ENCRYPT, "qcow");
+ } else if (val && !strcmp(val, "off")) {
+ qdict_del(qdict, BLOCK_OPT_ENCRYPT);
+ }
+
+ val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT);
+ if (val && !strcmp(val, "aes")) {
+ qdict_put_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT, "qcow");
+ }
+
+ if (!qdict_rename_keys(qdict, opt_renames, errp)) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Create and open the file (protocol layer) */
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto fail;
+ }
+
+ /* Now get the QAPI type BlockdevCreateOptions */
+ qdict_put_str(qdict, "driver", "qcow");
+ qdict_put_str(qdict, "file", bs->node_name);
+
+ qobj = qdict_crumple(qdict, errp);
+ QDECREF(qdict);
+ qdict = qobject_to_qdict(qobj);
+ if (qdict == NULL) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+ visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+ visit_free(v);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Silently round up size */
+ assert(create_options->driver == BLOCKDEV_DRIVER_QCOW);
+ create_options->u.qcow.size =
+ ROUND_UP(create_options->u.qcow.size, BDRV_SECTOR_SIZE);
+
+ /* Create the qcow image (format layer) */
+ ret = qcow_co_create(create_options, errp);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ ret = 0;
+fail:
+ QDECREF(qdict);
+ bdrv_unref(bs);
+ qapi_free_BlockdevCreateOptions(create_options);
return ret;
}
@@ -1128,6 +1191,7 @@ static BlockDriver bdrv_qcow = {
.bdrv_close = qcow_close,
.bdrv_child_perm = bdrv_format_default_perms,
.bdrv_reopen_prepare = qcow_reopen_prepare,
+ .bdrv_co_create = qcow_co_create,
.bdrv_co_create_opts = qcow_co_create_opts,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.supports_backing = true,
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create Kevin Wolf
@ 2018-03-09 21:58 ` Eric Blake
2018-03-12 12:20 ` Kevin Wolf
2018-03-12 16:49 ` Max Reitz
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Eric Blake @ 2018-03-09 21:58 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, den, jcody, qemu-devel
On 03/09/2018 03:46 PM, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qcow, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 21 +++++-
> block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 150 insertions(+), 67 deletions(-)
Pre-review question: do we REALLY want to support creation of new qcow
images from QMP? Or are we at the point where we want to declare qcow a
read-only format where we only support it to the extent that you can
convert an existing qcow file into a better supported format like qcow2?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-09 21:58 ` Eric Blake
@ 2018-03-12 12:20 ` Kevin Wolf
0 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2018-03-12 12:20 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-block, mreitz, den, jcody, qemu-devel
Am 09.03.2018 um 22:58 hat Eric Blake geschrieben:
> On 03/09/2018 03:46 PM, Kevin Wolf wrote:
> > This adds the .bdrv_co_create driver callback to qcow, which
> > enables image creation over QMP.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/block-core.json | 21 +++++-
> > block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
> > 2 files changed, 150 insertions(+), 67 deletions(-)
>
> Pre-review question: do we REALLY want to support creation of new qcow
> images from QMP? Or are we at the point where we want to declare qcow
> a read-only format where we only support it to the extent that you can
> convert an existing qcow file into a better supported format like
> qcow2?
I don't think we want read-only formats if it can be avoided, because
we're in a much worse position to run tests then.
The other option you mentioned in your reply to the qed patch, just not
implementing .bdrv_co_create, but keeping the old callback, would mean
that we'd be stuck in a half-converted state forever. My goal is to get
rid of .bdrv_co_create_opts in the long run.
And actually, qcow and qed were two of the simpler conversions where
little remains to be done before the logic in .bdrv_co_create_opts can
be generalised in block.c.
So I'd just do the conversion.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create Kevin Wolf
2018-03-09 21:58 ` Eric Blake
@ 2018-03-12 16:49 ` Max Reitz
2018-03-12 21:31 ` Jeff Cody
2018-03-14 11:16 ` Eric Blake
3 siblings, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-12 16:49 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 464 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qcow, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 21 +++++-
> block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 150 insertions(+), 67 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
(still not sure about the crumpling)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create Kevin Wolf
2018-03-09 21:58 ` Eric Blake
2018-03-12 16:49 ` Max Reitz
@ 2018-03-12 21:31 ` Jeff Cody
2018-03-14 11:16 ` Eric Blake
3 siblings, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2018-03-12 21:31 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, den, eblake, qemu-devel
On Fri, Mar 09, 2018 at 10:46:07PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qcow, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 21 +++++-
> block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 150 insertions(+), 67 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d38058eeab..c81677c434 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3497,6 +3497,25 @@
> '*cluster-size': 'size' } }
>
> ##
> +# @BlockdevCreateOptionsQcow:
> +#
> +# Driver specific image creation options for qcow.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @backing-file File name of the backing file if a backing file
> +# should be used
> +# @encrypt Encryption options if the image should be encrypted
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsQcow',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*backing-file': 'str',
> + '*encrypt': 'QCryptoBlockCreateOptions' } }
> +
> +##
> # @BlockdevQcow2Version:
> #
> # @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2)
> @@ -3681,8 +3700,8 @@
> 'null-co': 'BlockdevCreateNotSupported',
> 'nvme': 'BlockdevCreateNotSupported',
> 'parallels': 'BlockdevCreateOptionsParallels',
> + 'qcow': 'BlockdevCreateOptionsQcow',
> 'qcow2': 'BlockdevCreateOptionsQcow2',
> - 'qcow': 'BlockdevCreateNotSupported',
> 'qed': 'BlockdevCreateNotSupported',
> 'quorum': 'BlockdevCreateNotSupported',
> 'raw': 'BlockdevCreateNotSupported',
> diff --git a/block/qcow.c b/block/qcow.c
> index 47a18d9a3a..2e3770ca63 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -33,6 +33,8 @@
> #include <zlib.h>
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qstring.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qapi-visit-block-core.h"
> #include "crypto/block.h"
> #include "migration/blocker.h"
> #include "block/crypto.h"
> @@ -86,6 +88,8 @@ typedef struct BDRVQcowState {
> Error *migration_blocker;
> } BDRVQcowState;
>
> +static QemuOptsList qcow_create_opts;
> +
> static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
>
> static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
> @@ -810,62 +814,50 @@ static void qcow_close(BlockDriverState *bs)
> error_free(s->migration_blocker);
> }
>
> -static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts,
> - Error **errp)
> +static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
> + Error **errp)
> {
> + BlockdevCreateOptionsQcow *qcow_opts;
> int header_size, backing_filename_len, l1_size, shift, i;
> QCowHeader header;
> uint8_t *tmp;
> int64_t total_size = 0;
> - char *backing_file = NULL;
> - Error *local_err = NULL;
> int ret;
> + BlockDriverState *bs;
> BlockBackend *qcow_blk;
> - char *encryptfmt = NULL;
> - QDict *options;
> - QDict *encryptopts = NULL;
> - QCryptoBlockCreateOptions *crypto_opts = NULL;
> QCryptoBlock *crypto = NULL;
>
> - /* Read out options */
> - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> - BDRV_SECTOR_SIZE);
> + assert(opts->driver == BLOCKDEV_DRIVER_QCOW);
> + qcow_opts = &opts->u.qcow;
> +
> + /* Sanity checks */
> + total_size = qcow_opts->size;
> if (total_size == 0) {
> error_setg(errp, "Image size is too small, cannot be zero length");
> - ret = -EINVAL;
> - goto cleanup;
> + return -EINVAL;
> }
>
> - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
> - encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
> - if (encryptfmt) {
> - if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
> - error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
> - BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
> - ret = -EINVAL;
> - goto cleanup;
> - }
> - } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
> - encryptfmt = g_strdup("aes");
> + if (qcow_opts->has_encrypt &&
> + qcow_opts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_QCOW)
> + {
> + error_setg(errp, "Unsupported encryption format");
> + return -EINVAL;
> }
>
> - ret = bdrv_create_file(filename, opts, &local_err);
> - if (ret < 0) {
> - error_propagate(errp, local_err);
> - goto cleanup;
> + /* Create BlockBackend to write to the image */
> + bs = bdrv_open_blockdev_ref(qcow_opts->file, errp);
> + if (bs == NULL) {
> + return -EIO;
> }
>
> - qcow_blk = blk_new_open(filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> - &local_err);
> - if (qcow_blk == NULL) {
> - error_propagate(errp, local_err);
> - ret = -EIO;
> - goto cleanup;
> + qcow_blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
> + ret = blk_insert_bs(qcow_blk, bs, errp);
> + if (ret < 0) {
> + goto exit;
> }
> -
> blk_set_allow_write_beyond_eof(qcow_blk, true);
>
> + /* Create image format */
> ret = blk_truncate(qcow_blk, 0, PREALLOC_MODE_OFF, errp);
> if (ret < 0) {
> goto exit;
> @@ -877,16 +869,15 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
> header.size = cpu_to_be64(total_size);
> header_size = sizeof(header);
> backing_filename_len = 0;
> - if (backing_file) {
> - if (strcmp(backing_file, "fat:")) {
> + if (qcow_opts->has_backing_file) {
> + if (strcmp(qcow_opts->backing_file, "fat:")) {
> header.backing_file_offset = cpu_to_be64(header_size);
> - backing_filename_len = strlen(backing_file);
> + backing_filename_len = strlen(qcow_opts->backing_file);
> header.backing_file_size = cpu_to_be32(backing_filename_len);
> header_size += backing_filename_len;
> } else {
> /* special backing file for vvfat */
> - g_free(backing_file);
> - backing_file = NULL;
> + qcow_opts->has_backing_file = false;
> }
> header.cluster_bits = 9; /* 512 byte cluster to avoid copying
> unmodified sectors */
> @@ -901,26 +892,10 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
>
> header.l1_table_offset = cpu_to_be64(header_size);
>
> - options = qemu_opts_to_qdict(opts, NULL);
> - qdict_extract_subqdict(options, &encryptopts, "encrypt.");
> - QDECREF(options);
> - if (encryptfmt) {
> - if (!g_str_equal(encryptfmt, "aes")) {
> - error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
> - encryptfmt);
> - ret = -EINVAL;
> - goto exit;
> - }
> + if (qcow_opts->has_encrypt) {
> header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
>
> - crypto_opts = block_crypto_create_opts_init(
> - Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
> - if (!crypto_opts) {
> - ret = -EINVAL;
> - goto exit;
> - }
> -
> - crypto = qcrypto_block_create(crypto_opts, "encrypt.",
> + crypto = qcrypto_block_create(qcow_opts->encrypt, "encrypt.",
> NULL, NULL, NULL, errp);
> if (!crypto) {
> ret = -EINVAL;
> @@ -936,9 +911,9 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
> goto exit;
> }
>
> - if (backing_file) {
> + if (qcow_opts->has_backing_file) {
> ret = blk_pwrite(qcow_blk, sizeof(header),
> - backing_file, backing_filename_len, 0);
> + qcow_opts->backing_file, backing_filename_len, 0);
> if (ret != backing_filename_len) {
> goto exit;
> }
> @@ -959,12 +934,100 @@ static int coroutine_fn qcow_co_create_opts(const char *filename, QemuOpts *opts
> ret = 0;
> exit:
> blk_unref(qcow_blk);
> -cleanup:
> - QDECREF(encryptopts);
> - g_free(encryptfmt);
> qcrypto_block_free(crypto);
> - qapi_free_QCryptoBlockCreateOptions(crypto_opts);
> - g_free(backing_file);
> + return ret;
> +}
> +
> +static int coroutine_fn qcow_co_create_opts(const char *filename,
> + QemuOpts *opts, Error **errp)
> +{
> + BlockdevCreateOptions *create_options = NULL;
> + BlockDriverState *bs = NULL;
> + QDict *qdict = NULL;
> + QObject *qobj;
> + Visitor *v;
> + const char *val;
> + Error *local_err = NULL;
> + int ret;
> +
> + static const QDictRenames opt_renames[] = {
> + { BLOCK_OPT_BACKING_FILE, "backing-file" },
> + { BLOCK_OPT_ENCRYPT, BLOCK_OPT_ENCRYPT_FORMAT },
> + { NULL, NULL },
> + };
> +
> + /* Parse options and convert legacy syntax */
> + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qcow_create_opts, true);
> +
> + val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
> + if (val && !strcmp(val, "on")) {
> + qdict_put_str(qdict, BLOCK_OPT_ENCRYPT, "qcow");
> + } else if (val && !strcmp(val, "off")) {
> + qdict_del(qdict, BLOCK_OPT_ENCRYPT);
> + }
> +
> + val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT);
> + if (val && !strcmp(val, "aes")) {
> + qdict_put_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT, "qcow");
> + }
> +
> + if (!qdict_rename_keys(qdict, opt_renames, errp)) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Create and open the file (protocol layer) */
> + ret = bdrv_create_file(filename, opts, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> +
> + bs = bdrv_open(filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
> + if (bs == NULL) {
> + ret = -EIO;
> + goto fail;
> + }
> +
> + /* Now get the QAPI type BlockdevCreateOptions */
> + qdict_put_str(qdict, "driver", "qcow");
> + qdict_put_str(qdict, "file", bs->node_name);
> +
> + qobj = qdict_crumple(qdict, errp);
> + QDECREF(qdict);
> + qdict = qobject_to_qdict(qobj);
> + if (qdict == NULL) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
> + visit_free(v);
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Silently round up size */
> + assert(create_options->driver == BLOCKDEV_DRIVER_QCOW);
> + create_options->u.qcow.size =
> + ROUND_UP(create_options->u.qcow.size, BDRV_SECTOR_SIZE);
> +
> + /* Create the qcow image (format layer) */
> + ret = qcow_co_create(create_options, errp);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> + ret = 0;
> +fail:
> + QDECREF(qdict);
> + bdrv_unref(bs);
> + qapi_free_BlockdevCreateOptions(create_options);
> return ret;
> }
>
> @@ -1128,6 +1191,7 @@ static BlockDriver bdrv_qcow = {
> .bdrv_close = qcow_close,
> .bdrv_child_perm = bdrv_format_default_perms,
> .bdrv_reopen_prepare = qcow_reopen_prepare,
> + .bdrv_co_create = qcow_co_create,
> .bdrv_co_create_opts = qcow_co_create_opts,
> .bdrv_has_zero_init = bdrv_has_zero_init_1,
> .supports_backing = true,
> --
> 2.13.6
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create Kevin Wolf
` (2 preceding siblings ...)
2018-03-12 21:31 ` Jeff Cody
@ 2018-03-14 11:16 ` Eric Blake
2018-03-14 11:19 ` Daniel P. Berrangé
3 siblings, 1 reply; 26+ messages in thread
From: Eric Blake @ 2018-03-14 11:16 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, den, jcody, qemu-devel
On 03/09/2018 03:46 PM, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qcow, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 21 +++++-
> block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 150 insertions(+), 67 deletions(-)
> ##
> +# @BlockdevCreateOptionsQcow:
> +#
> +# Driver specific image creation options for qcow.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @backing-file File name of the backing file if a backing file
> +# should be used
> +# @encrypt Encryption options if the image should be encrypted
Idea for followup patch - we should strongly document that encryption of
qcow is discouraged as insecure, and/or mention that qcow2 is generally
a better option than qcow when creating images over QMP.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create
2018-03-14 11:16 ` Eric Blake
@ 2018-03-14 11:19 ` Daniel P. Berrangé
0 siblings, 0 replies; 26+ messages in thread
From: Daniel P. Berrangé @ 2018-03-14 11:19 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-block, den, jcody, qemu-devel, mreitz
On Wed, Mar 14, 2018 at 06:16:18AM -0500, Eric Blake wrote:
> On 03/09/2018 03:46 PM, Kevin Wolf wrote:
> > This adds the .bdrv_co_create driver callback to qcow, which
> > enables image creation over QMP.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/block-core.json | 21 +++++-
> > block/qcow.c | 196 ++++++++++++++++++++++++++++++++++-----------------
> > 2 files changed, 150 insertions(+), 67 deletions(-)
>
> > ##
> > +# @BlockdevCreateOptionsQcow:
> > +#
> > +# Driver specific image creation options for qcow.
> > +#
> > +# @file Node to create the image format on
> > +# @size Size of the virtual disk in bytes
> > +# @backing-file File name of the backing file if a backing file
> > +# should be used
> > +# @encrypt Encryption options if the image should be encrypted
>
> Idea for followup patch - we should strongly document that encryption of
> qcow is discouraged as insecure, and/or mention that qcow2 is generally a
> better option than qcow when creating images over QMP.
Yes to the encryption note, but we should definitely document that
'qcow' is deprecated in general - there's little good reason you would
want to use it - it has terrible performance when allocating new clusters.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 4/7] qed: Support .bdrv_co_create
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
` (2 preceding siblings ...)
2018-03-09 21:46 ` [Qemu-devel] [PATCH 3/7] qcow: Support .bdrv_co_create Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-09 22:01 ` Eric Blake
2018-03-12 21:20 ` Max Reitz
2018-03-09 21:46 ` [Qemu-devel] [PATCH 5/7] vdi: " Kevin Wolf
` (2 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
This adds the .bdrv_co_create driver callback to qed, which
enables image creation over QMP.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 25 ++++++-
block/qed.c | 204 ++++++++++++++++++++++++++++++++++-----------------
2 files changed, 162 insertions(+), 67 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c81677c434..1e2edbc063 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3559,6 +3559,29 @@
'*refcount-bits': 'int' } }
##
+# @BlockdevCreateOptionsQed:
+#
+# Driver specific image creation options for qed.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @backing-file File name of the backing file if a backing file
+# should be used
+# @backing-fmt Name of the block driver to use for the backing file
+# @cluster-size Cluster size in bytes (default: 65536)
+# @table-size L1/L2 table size (in clusters)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsQed',
+ 'data': { 'file': 'BlockdevRef',
+ 'size': 'size',
+ '*backing-file': 'str',
+ '*backing-fmt': 'BlockdevDriver',
+ '*cluster-size': 'size',
+ '*table-size': 'int' } }
+
+##
# @BlockdevCreateOptionsRbd:
#
# Driver specific image creation options for rbd/Ceph.
@@ -3702,7 +3725,7 @@
'parallels': 'BlockdevCreateOptionsParallels',
'qcow': 'BlockdevCreateOptionsQcow',
'qcow2': 'BlockdevCreateOptionsQcow2',
- 'qed': 'BlockdevCreateNotSupported',
+ 'qed': 'BlockdevCreateOptionsQed',
'quorum': 'BlockdevCreateNotSupported',
'raw': 'BlockdevCreateNotSupported',
'rbd': 'BlockdevCreateOptionsRbd',
diff --git a/block/qed.c b/block/qed.c
index 5e6a6bfaa0..46a84beeed 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -20,6 +20,11 @@
#include "trace.h"
#include "qed.h"
#include "sysemu/block-backend.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-block-core.h"
+
+static QemuOptsList qed_create_opts;
static int bdrv_qed_probe(const uint8_t *buf, int buf_size,
const char *filename)
@@ -594,57 +599,95 @@ static void bdrv_qed_close(BlockDriverState *bs)
qemu_vfree(s->l1_table);
}
-static int qed_create(const char *filename, uint32_t cluster_size,
- uint64_t image_size, uint32_t table_size,
- const char *backing_file, const char *backing_fmt,
- QemuOpts *opts, Error **errp)
+static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
+ Error **errp)
{
- QEDHeader header = {
- .magic = QED_MAGIC,
- .cluster_size = cluster_size,
- .table_size = table_size,
- .header_size = 1,
- .features = 0,
- .compat_features = 0,
- .l1_table_offset = cluster_size,
- .image_size = image_size,
- };
+ BlockdevCreateOptionsQed *qed_opts;
+ BlockBackend *blk = NULL;
+ BlockDriverState *bs = NULL;
+
+ QEDHeader header;
QEDHeader le_header;
uint8_t *l1_table = NULL;
- size_t l1_size = header.cluster_size * header.table_size;
- Error *local_err = NULL;
+ size_t l1_size;
int ret = 0;
- BlockBackend *blk;
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- return ret;
+ assert(opts->driver == BLOCKDEV_DRIVER_QED);
+ qed_opts = &opts->u.qed;
+
+ /* Validate options and set default values */
+ if (!qed_opts->has_cluster_size) {
+ qed_opts->cluster_size = QED_DEFAULT_CLUSTER_SIZE;
+ }
+ if (!qed_opts->has_table_size) {
+ qed_opts->table_size = QED_DEFAULT_TABLE_SIZE;
}
- blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (blk == NULL) {
- error_propagate(errp, local_err);
+ if (!qed_is_cluster_size_valid(qed_opts->cluster_size)) {
+ error_setg(errp, "QED cluster size must be within range [%u, %u] "
+ "and power of 2",
+ QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE);
+ return -EINVAL;
+ }
+ if (!qed_is_table_size_valid(qed_opts->table_size)) {
+ error_setg(errp, "QED table size must be within range [%u, %u] "
+ "and power of 2",
+ QED_MIN_TABLE_SIZE, QED_MAX_TABLE_SIZE);
+ return -EINVAL;
+ }
+ if (!qed_is_image_size_valid(qed_opts->size, qed_opts->cluster_size,
+ qed_opts->table_size))
+ {
+ error_setg(errp, "QED image size must be a non-zero multiple of "
+ "cluster size and less than %" PRIu64 " bytes",
+ qed_max_image_size(qed_opts->cluster_size,
+ qed_opts->table_size));
+ return -EINVAL;
+ }
+
+ /* Create BlockBackend to write to the image */
+ bs = bdrv_open_blockdev_ref(qed_opts->file, errp);
+ if (bs == NULL) {
return -EIO;
}
+ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ ret = blk_insert_bs(blk, bs, errp);
+ if (ret < 0) {
+ goto out;
+ }
blk_set_allow_write_beyond_eof(blk, true);
+ /* Prepare image format */
+ header = (QEDHeader) {
+ .magic = QED_MAGIC,
+ .cluster_size = qed_opts->cluster_size,
+ .table_size = qed_opts->table_size,
+ .header_size = 1,
+ .features = 0,
+ .compat_features = 0,
+ .l1_table_offset = qed_opts->cluster_size,
+ .image_size = qed_opts->size,
+ };
+
+ l1_size = header.cluster_size * header.table_size;
+
/* File must start empty and grow, check truncate is supported */
ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
if (ret < 0) {
goto out;
}
- if (backing_file) {
+ if (qed_opts->has_backing_file) {
header.features |= QED_F_BACKING_FILE;
header.backing_filename_offset = sizeof(le_header);
- header.backing_filename_size = strlen(backing_file);
+ header.backing_filename_size = strlen(qed_opts->backing_file);
- if (qed_fmt_is_raw(backing_fmt)) {
- header.features |= QED_F_BACKING_FORMAT_NO_PROBE;
+ if (qed_opts->has_backing_fmt) {
+ const char *backing_fmt = BlockdevDriver_str(qed_opts->backing_fmt);
+ if (qed_fmt_is_raw(backing_fmt)) {
+ header.features |= QED_F_BACKING_FORMAT_NO_PROBE;
+ }
}
}
@@ -653,7 +696,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
if (ret < 0) {
goto out;
}
- ret = blk_pwrite(blk, sizeof(le_header), backing_file,
+ ret = blk_pwrite(blk, sizeof(le_header), qed_opts->backing_file,
header.backing_filename_size, 0);
if (ret < 0) {
goto out;
@@ -669,6 +712,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
out:
g_free(l1_table);
blk_unref(blk);
+ bdrv_unref(bs);
return ret;
}
@@ -676,51 +720,78 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
QemuOpts *opts,
Error **errp)
{
- uint64_t image_size = 0;
- uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE;
- uint32_t table_size = QED_DEFAULT_TABLE_SIZE;
- char *backing_file = NULL;
- char *backing_fmt = NULL;
+ BlockdevCreateOptions *create_options = NULL;
+ QDict *qdict = NULL;
+ QObject *qobj;
+ Visitor *v;
+ BlockDriverState *bs = NULL;
+ Error *local_err = NULL;
int ret;
- image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
- backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
- backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
- cluster_size = qemu_opt_get_size_del(opts,
- BLOCK_OPT_CLUSTER_SIZE,
- QED_DEFAULT_CLUSTER_SIZE);
- table_size = qemu_opt_get_size_del(opts, BLOCK_OPT_TABLE_SIZE,
- QED_DEFAULT_TABLE_SIZE);
-
- if (!qed_is_cluster_size_valid(cluster_size)) {
- error_setg(errp, "QED cluster size must be within range [%u, %u] "
- "and power of 2",
- QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE);
+ static const QDictRenames opt_renames[] = {
+ { BLOCK_OPT_BACKING_FILE, "backing-file" },
+ { BLOCK_OPT_BACKING_FMT, "backing-fmt" },
+ { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" },
+ { BLOCK_OPT_TABLE_SIZE, "table-size" },
+ { NULL, NULL },
+ };
+
+ /* Parse options and convert legacy syntax */
+ qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qed_create_opts, true);
+
+ if (!qdict_rename_keys(qdict, opt_renames, errp)) {
ret = -EINVAL;
- goto finish;
+ goto fail;
}
- if (!qed_is_table_size_valid(table_size)) {
- error_setg(errp, "QED table size must be within range [%u, %u] "
- "and power of 2",
- QED_MIN_TABLE_SIZE, QED_MAX_TABLE_SIZE);
+
+ /* Create and open the file (protocol layer) */
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto fail;
+ }
+
+ /* Now get the QAPI type BlockdevCreateOptions */
+ qdict_put_str(qdict, "driver", "qed");
+ qdict_put_str(qdict, "file", bs->node_name);
+
+ qobj = qdict_crumple(qdict, errp);
+ QDECREF(qdict);
+ qdict = qobject_to_qdict(qobj);
+ if (qdict == NULL) {
ret = -EINVAL;
- goto finish;
+ goto fail;
}
- if (!qed_is_image_size_valid(image_size, cluster_size, table_size)) {
- error_setg(errp, "QED image size must be a non-zero multiple of "
- "cluster size and less than %" PRIu64 " bytes",
- qed_max_image_size(cluster_size, table_size));
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+ visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+ visit_free(v);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
ret = -EINVAL;
- goto finish;
+ goto fail;
}
- ret = qed_create(filename, cluster_size, image_size, table_size,
- backing_file, backing_fmt, opts, errp);
+ /* Silently round up size */
+ assert(create_options->driver == BLOCKDEV_DRIVER_QED);
+ create_options->u.qed.size =
+ ROUND_UP(create_options->u.qed.size, BDRV_SECTOR_SIZE);
+
+ /* Create the qed image (format layer) */
+ ret = bdrv_qed_co_create(create_options, errp);
-finish:
- g_free(backing_file);
- g_free(backing_fmt);
+fail:
+ QDECREF(qdict);
+ bdrv_unref(bs);
+ qapi_free_BlockdevCreateOptions(create_options);
return ret;
}
@@ -1602,6 +1673,7 @@ static BlockDriver bdrv_qed = {
.bdrv_close = bdrv_qed_close,
.bdrv_reopen_prepare = bdrv_qed_reopen_prepare,
.bdrv_child_perm = bdrv_format_default_perms,
+ .bdrv_co_create = bdrv_qed_co_create,
.bdrv_co_create_opts = bdrv_qed_co_create_opts,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_block_status = bdrv_qed_co_block_status,
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] qed: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 4/7] qed: " Kevin Wolf
@ 2018-03-09 22:01 ` Eric Blake
2018-03-12 21:20 ` Max Reitz
1 sibling, 0 replies; 26+ messages in thread
From: Eric Blake @ 2018-03-09 22:01 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, den, jcody, qemu-devel
On 03/09/2018 03:46 PM, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qed, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 25 ++++++-
> block/qed.c | 204 ++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 162 insertions(+), 67 deletions(-)
Similar question as to qcow (who still creates qed images these days?
and if no one seriously does it outside of our testsuite, would it be
better to not allow QMP creation of qed images?). On the other hand,
qed is newer than qcow so it doesn't have quite the legacy of poor
usage, so it may also mean that qed gets a longer deprecation cycle than
qcow.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] qed: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 4/7] qed: " Kevin Wolf
2018-03-09 22:01 ` Eric Blake
@ 2018-03-12 21:20 ` Max Reitz
1 sibling, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-12 21:20 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 424 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to qed, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 25 ++++++-
> block/qed.c | 204 ++++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 162 insertions(+), 67 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 5/7] vdi: Support .bdrv_co_create
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
` (3 preceding siblings ...)
2018-03-09 21:46 ` [Qemu-devel] [PATCH 4/7] qed: " Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-12 21:22 ` Max Reitz
2018-03-09 21:46 ` [Qemu-devel] [PATCH 6/7] vhdx: " Kevin Wolf
2018-03-09 21:46 ` [Qemu-devel] [PATCH 7/7] vpc: " Kevin Wolf
6 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
This adds the .bdrv_co_create driver callback to vdi, which
enables image creation over QMP.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 21 ++++++-
block/vdi.c | 169 ++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 148 insertions(+), 42 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1e2edbc063..2eba0eef7e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3680,6 +3680,25 @@
'size': 'size' } }
##
+# @BlockdevCreateOptionsVdi:
+#
+# Driver specific image creation options for vdi.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @cluster-size Cluster size in bytes (default: 1 MB)
+# @static Whether to create a static (preallocated) image
+# (default: false)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsVdi',
+ 'data': { 'file': 'BlockdevRef',
+ 'size': 'size',
+ '*cluster-size': 'size',
+ '*static': 'bool' } }
+
+##
# @BlockdevCreateNotSupported:
#
# This is used for all drivers that don't support creating images.
@@ -3733,7 +3752,7 @@
'sheepdog': 'BlockdevCreateOptionsSheepdog',
'ssh': 'BlockdevCreateOptionsSsh',
'throttle': 'BlockdevCreateNotSupported',
- 'vdi': 'BlockdevCreateNotSupported',
+ 'vdi': 'BlockdevCreateOptionsVdi',
'vhdx': 'BlockdevCreateNotSupported',
'vmdk': 'BlockdevCreateNotSupported',
'vpc': 'BlockdevCreateNotSupported',
diff --git a/block/vdi.c b/block/vdi.c
index 2b5ddd0666..c60ddc58c0 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -60,6 +60,9 @@
#include "qemu/coroutine.h"
#include "qemu/cutils.h"
#include "qemu/uuid.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-block-core.h"
/* Code configuration options. */
@@ -182,6 +185,8 @@ typedef struct {
Error *migration_blocker;
} BDRVVdiState;
+static QemuOptsList vdi_create_opts;
+
static void vdi_header_to_cpu(VdiHeader *header)
{
le32_to_cpus(&header->signature);
@@ -716,67 +721,72 @@ nonallocating_write:
return ret;
}
-static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
+static int coroutine_fn vdi_co_create(BlockdevCreateOptions *opts,
+ Error **errp)
{
+ BlockdevCreateOptionsVdi *vdi_opts;
+ BlockBackend *blk = NULL;
+ BlockDriverState *bs = NULL;
+
int ret = 0;
- uint64_t bytes = 0;
uint32_t blocks;
- size_t block_size = DEFAULT_CLUSTER_SIZE;
uint32_t image_type = VDI_TYPE_DYNAMIC;
VdiHeader header;
size_t i;
size_t bmap_size;
int64_t offset = 0;
- Error *local_err = NULL;
- BlockBackend *blk = NULL;
uint32_t *bmap = NULL;
logout("\n");
- /* Read out options. */
- bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
-#if defined(CONFIG_VDI_BLOCK_SIZE)
- /* TODO: Additional checks (SECTOR_SIZE * 2^n, ...). */
- block_size = qemu_opt_get_size_del(opts,
- BLOCK_OPT_CLUSTER_SIZE,
- DEFAULT_CLUSTER_SIZE);
-#endif
-#if defined(CONFIG_VDI_STATIC_IMAGE)
- if (qemu_opt_get_bool_del(opts, BLOCK_OPT_STATIC, false)) {
- image_type = VDI_TYPE_STATIC;
+ assert(opts->driver == BLOCKDEV_DRIVER_VDI);
+ vdi_opts = &opts->u.vdi;
+
+ /* Validate options and set default values */
+ if (!vdi_opts->has_cluster_size) {
+ vdi_opts->cluster_size = DEFAULT_CLUSTER_SIZE;
}
-#endif
- if (bytes > VDI_DISK_SIZE_MAX) {
- ret = -ENOTSUP;
+ if (vdi_opts->size > VDI_DISK_SIZE_MAX) {
error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
", max supported is 0x%" PRIx64 ")",
- bytes, VDI_DISK_SIZE_MAX);
- goto exit;
+ vdi_opts->size, VDI_DISK_SIZE_MAX);
+ return -ENOTSUP;
}
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- goto exit;
+#if !defined(CONFIG_VDI_BLOCK_SIZE)
+ if (vdi_opts->has_cluster_size) {
+ error_setg(errp, "Non-default cluster size not supported");
+ return -ENOTSUP;
+ }
+#endif
+#if !defined(CONFIG_VDI_STATIC_IMAGE)
+ if (vdi_opts->has_static) {
+ error_setg(errp, "Static images not supported");
+ return -ENOTSUP;
+ }
+#else
+ if (vdi_opts->q_static) {
+ image_type = VDI_TYPE_STATIC;
+ }
+#endif
+
+ /* Create BlockBackend to write to the image */
+ bs = bdrv_open_blockdev_ref(vdi_opts->file, errp);
+ if (bs == NULL) {
+ return -EIO;
}
- blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (blk == NULL) {
- error_propagate(errp, local_err);
- ret = -EIO;
+ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ ret = blk_insert_bs(blk, bs, errp);
+ if (ret < 0) {
goto exit;
}
-
blk_set_allow_write_beyond_eof(blk, true);
/* We need enough blocks to store the given disk size,
so always round up. */
- blocks = DIV_ROUND_UP(bytes, block_size);
+ blocks = DIV_ROUND_UP(vdi_opts->size, vdi_opts->cluster_size);
bmap_size = blocks * sizeof(uint32_t);
bmap_size = ROUND_UP(bmap_size, SECTOR_SIZE);
@@ -790,8 +800,8 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
header.offset_bmap = 0x200;
header.offset_data = 0x200 + bmap_size;
header.sector_size = SECTOR_SIZE;
- header.disk_size = bytes;
- header.block_size = block_size;
+ header.disk_size = vdi_opts->size;
+ header.block_size = vdi_opts->cluster_size;
header.blocks_in_image = blocks;
if (image_type == VDI_TYPE_STATIC) {
header.blocks_allocated = blocks;
@@ -805,7 +815,7 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
vdi_header_to_le(&header);
ret = blk_pwrite(blk, offset, &header, sizeof(header), 0);
if (ret < 0) {
- error_setg(errp, "Error writing header to %s", filename);
+ error_setg(errp, "Error writing header");
goto exit;
}
offset += sizeof(header);
@@ -826,27 +836,103 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
}
ret = blk_pwrite(blk, offset, bmap, bmap_size, 0);
if (ret < 0) {
- error_setg(errp, "Error writing bmap to %s", filename);
+ error_setg(errp, "Error writing bmap");
goto exit;
}
offset += bmap_size;
}
if (image_type == VDI_TYPE_STATIC) {
- ret = blk_truncate(blk, offset + blocks * block_size,
+ ret = blk_truncate(blk, offset + blocks * vdi_opts->cluster_size,
PREALLOC_MODE_OFF, errp);
if (ret < 0) {
- error_prepend(errp, "Failed to statically allocate %s", filename);
+ error_prepend(errp, "Failed to statically allocate image");
goto exit;
}
}
exit:
blk_unref(blk);
+ bdrv_unref(bs);
g_free(bmap);
return ret;
}
+static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
+ Error **errp)
+{
+ BlockdevCreateOptions *create_options = NULL;
+ QDict *qdict = NULL;
+ QObject *qobj;
+ Visitor *v;
+ BlockDriverState *bs = NULL;
+ Error *local_err = NULL;
+ int ret;
+
+ static const QDictRenames opt_renames[] = {
+ { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" },
+ { NULL, NULL },
+ };
+
+ /* Parse options and convert legacy syntax */
+ qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true);
+
+ if (!qdict_rename_keys(qdict, opt_renames, errp)) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Create and open the file (protocol layer) */
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto fail;
+ }
+
+ /* Now get the QAPI type BlockdevCreateOptions */
+ qdict_put_str(qdict, "driver", "vdi");
+ qdict_put_str(qdict, "file", bs->node_name);
+
+ qobj = qdict_crumple(qdict, errp);
+ QDECREF(qdict);
+ qdict = qobject_to_qdict(qobj);
+ if (qdict == NULL) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+ visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+ visit_free(v);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Silently round up size */
+ assert(create_options->driver == BLOCKDEV_DRIVER_VDI);
+ create_options->u.vdi.size =
+ ROUND_UP(create_options->u.vdi.size, BDRV_SECTOR_SIZE);
+
+ /* Create the vdi image (format layer) */
+ ret = vdi_co_create(create_options, errp);
+
+fail:
+ QDECREF(qdict);
+ bdrv_unref(bs);
+ qapi_free_BlockdevCreateOptions(create_options);
+ return ret;
+}
+
static void vdi_close(BlockDriverState *bs)
{
BDRVVdiState *s = bs->opaque;
@@ -895,6 +981,7 @@ static BlockDriver bdrv_vdi = {
.bdrv_close = vdi_close,
.bdrv_reopen_prepare = vdi_reopen_prepare,
.bdrv_child_perm = bdrv_format_default_perms,
+ .bdrv_co_create = vdi_co_create,
.bdrv_co_create_opts = vdi_co_create_opts,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_block_status = vdi_co_block_status,
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] vdi: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 5/7] vdi: " Kevin Wolf
@ 2018-03-12 21:22 ` Max Reitz
0 siblings, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-12 21:22 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 624 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to vdi, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 21 ++++++-
> block/vdi.c | 169 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 148 insertions(+), 42 deletions(-)
As I said on IRC, I'd prefer cluster-size not to be exposed over QAPI,
seeing it is not supported by default (and you can't enable it through
configure, you can only enable it by modifying vdi.c), and that it is
explicitly untested.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 6/7] vhdx: Support .bdrv_co_create
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
` (4 preceding siblings ...)
2018-03-09 21:46 ` [Qemu-devel] [PATCH 5/7] vdi: " Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-12 19:37 ` Jeff Cody
2018-03-12 21:38 ` Max Reitz
2018-03-09 21:46 ` [Qemu-devel] [PATCH 7/7] vpc: " Kevin Wolf
6 siblings, 2 replies; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
This adds the .bdrv_co_create driver callback to vhdx, which
enables image creation over QMP.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 37 ++++++++++-
block/vhdx.c | 174 ++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 167 insertions(+), 44 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2eba0eef7e..3a65909c47 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3699,6 +3699,41 @@
'*static': 'bool' } }
##
+# @BlockdevVhdxSubformat:
+#
+# @dynamic: Growing image file
+# @fixed: Preallocated fixed-size imge file
+#
+# Since: 2.12
+##
+{ 'enum': 'BlockdevVhdxSubformat',
+ 'data': [ 'dynamic', 'fixed' ] }
+
+##
+# @BlockdevCreateOptionsVhdx:
+#
+# Driver specific image creation options for vhdx.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @log-size Log size in bytes (default: 1 MB)
+# @block-size Block size in bytes (default: 1 MB)
+# @subformat vhdx subformat (default: dynamic)
+# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard,
+# but default. Do not set to 'off' when using 'qemu-img
+# convert' with subformat=dynamic.
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsVhdx',
+ 'data': { 'file': 'BlockdevRef',
+ 'size': 'size',
+ '*log-size': 'size',
+ '*block-size': 'size',
+ '*subformat': 'BlockdevVhdxSubformat',
+ '*block-state-zero': 'bool' } }
+
+##
# @BlockdevCreateNotSupported:
#
# This is used for all drivers that don't support creating images.
@@ -3753,7 +3788,7 @@
'ssh': 'BlockdevCreateOptionsSsh',
'throttle': 'BlockdevCreateNotSupported',
'vdi': 'BlockdevCreateOptionsVdi',
- 'vhdx': 'BlockdevCreateNotSupported',
+ 'vhdx': 'BlockdevCreateOptionsVhdx',
'vmdk': 'BlockdevCreateNotSupported',
'vpc': 'BlockdevCreateNotSupported',
'vvfat': 'BlockdevCreateNotSupported',
diff --git a/block/vhdx.c b/block/vhdx.c
index d82350d07c..0ce972381f 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -26,6 +26,9 @@
#include "block/vhdx.h"
#include "migration/blocker.h"
#include "qemu/uuid.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-block-core.h"
/* Options for VHDX creation */
@@ -39,6 +42,8 @@ typedef enum VHDXImageType {
VHDX_TYPE_DIFFERENCING, /* Currently unsupported */
} VHDXImageType;
+static QemuOptsList vhdx_create_opts;
+
/* Several metadata and region table data entries are identified by
* guids in a MS-specific GUID format. */
@@ -1792,54 +1797,63 @@ exit:
* .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------.
* 1MB
*/
-static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
+static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
+ Error **errp)
{
+ BlockdevCreateOptionsVhdx *vhdx_opts;
+ BlockBackend *blk = NULL;
+ BlockDriverState *bs = NULL;
+
int ret = 0;
- uint64_t image_size = (uint64_t) 2 * GiB;
- uint32_t log_size = 1 * MiB;
- uint32_t block_size = 0;
+ uint64_t image_size;
+ uint32_t log_size;
+ uint32_t block_size;
uint64_t signature;
uint64_t metadata_offset;
bool use_zero_blocks = false;
gunichar2 *creator = NULL;
glong creator_items;
- BlockBackend *blk;
- char *type = NULL;
VHDXImageType image_type;
- Error *local_err = NULL;
- image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
- log_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_LOG_SIZE, 0);
- block_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_BLOCK_SIZE, 0);
- type = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
- use_zero_blocks = qemu_opt_get_bool_del(opts, VHDX_BLOCK_OPT_ZERO, true);
+ assert(opts->driver == BLOCKDEV_DRIVER_VHDX);
+ vhdx_opts = &opts->u.vhdx;
+
+ /* Validate options and set default values */
+
+ image_size = vhdx_opts->size;
+ block_size = vhdx_opts->block_size;
+
+ if (!vhdx_opts->has_log_size) {
+ log_size = DEFAULT_LOG_SIZE;
+ } else {
+ log_size = vhdx_opts->log_size;
+ }
+
+ if (!vhdx_opts->has_block_state_zero) {
+ use_zero_blocks = true;
+ } else {
+ use_zero_blocks = vhdx_opts->block_state_zero;
+ }
if (image_size > VHDX_MAX_IMAGE_SIZE) {
error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB");
- ret = -EINVAL;
- goto exit;
+ return -EINVAL;
}
- if (type == NULL) {
- type = g_strdup("dynamic");
+ if (!vhdx_opts->has_subformat) {
+ vhdx_opts->subformat = BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC;
}
- if (!strcmp(type, "dynamic")) {
+ switch (vhdx_opts->subformat) {
+ case BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC:
image_type = VHDX_TYPE_DYNAMIC;
- } else if (!strcmp(type, "fixed")) {
+ break;
+ case BLOCKDEV_VHDX_SUBFORMAT_FIXED:
image_type = VHDX_TYPE_FIXED;
- } else if (!strcmp(type, "differencing")) {
- error_setg_errno(errp, ENOTSUP,
- "Differencing files not yet supported");
- ret = -ENOTSUP;
- goto exit;
- } else {
- error_setg(errp, "Invalid subformat '%s'", type);
- ret = -EINVAL;
- goto exit;
+ break;
+ default:
+ g_assert_not_reached();
}
/* These are pretty arbitrary, and mainly designed to keep the BAT
@@ -1865,21 +1879,17 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts
block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX :
block_size;
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- goto exit;
+ /* Create BlockBackend to write to the image */
+ bs = bdrv_open_blockdev_ref(vhdx_opts->file, errp);
+ if (bs == NULL) {
+ return -EIO;
}
- blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (blk == NULL) {
- error_propagate(errp, local_err);
- ret = -EIO;
- goto exit;
+ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ ret = blk_insert_bs(blk, bs, errp);
+ if (ret < 0) {
+ goto delete_and_exit;
}
-
blk_set_allow_write_beyond_eof(blk, true);
/* Create (A) */
@@ -1931,12 +1941,89 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts
delete_and_exit:
blk_unref(blk);
-exit:
- g_free(type);
+ bdrv_unref(bs);
g_free(creator);
return ret;
}
+static int coroutine_fn vhdx_co_create_opts(const char *filename,
+ QemuOpts *opts,
+ Error **errp)
+{
+ BlockdevCreateOptions *create_options = NULL;
+ QDict *qdict = NULL;
+ QObject *qobj;
+ Visitor *v;
+ BlockDriverState *bs = NULL;
+ Error *local_err = NULL;
+ int ret;
+
+ static const QDictRenames opt_renames[] = {
+ { VHDX_BLOCK_OPT_LOG_SIZE, "log-size" },
+ { VHDX_BLOCK_OPT_BLOCK_SIZE, "block-size" },
+ { VHDX_BLOCK_OPT_ZERO, "block-state-zero" },
+ { NULL, NULL },
+ };
+
+ /* Parse options and convert legacy syntax */
+ qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vhdx_create_opts, true);
+
+ if (!qdict_rename_keys(qdict, opt_renames, errp)) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Create and open the file (protocol layer) */
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto fail;
+ }
+
+ /* Now get the QAPI type BlockdevCreateOptions */
+ qdict_put_str(qdict, "driver", "vhdx");
+ qdict_put_str(qdict, "file", bs->node_name);
+
+ qobj = qdict_crumple(qdict, errp);
+ QDECREF(qdict);
+ qdict = qobject_to_qdict(qobj);
+ if (qdict == NULL) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+ visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+ visit_free(v);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Silently round up size */
+ assert(create_options->driver == BLOCKDEV_DRIVER_VHDX);
+ create_options->u.vhdx.size =
+ ROUND_UP(create_options->u.vhdx.size, BDRV_SECTOR_SIZE);
+
+ /* Create the vhdx image (format layer) */
+ ret = vhdx_co_create(create_options, errp);
+
+fail:
+ QDECREF(qdict);
+ bdrv_unref(bs);
+ qapi_free_BlockdevCreateOptions(create_options);
+ return ret;
+}
+
/* If opened r/w, the VHDX driver will automatically replay the log,
* if one is present, inside the vhdx_open() call.
*
@@ -2005,6 +2092,7 @@ static BlockDriver bdrv_vhdx = {
.bdrv_child_perm = bdrv_format_default_perms,
.bdrv_co_readv = vhdx_co_readv,
.bdrv_co_writev = vhdx_co_writev,
+ .bdrv_co_create = vhdx_co_create,
.bdrv_co_create_opts = vhdx_co_create_opts,
.bdrv_get_info = vhdx_get_info,
.bdrv_co_check = vhdx_co_check,
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] vhdx: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 6/7] vhdx: " Kevin Wolf
@ 2018-03-12 19:37 ` Jeff Cody
2018-03-12 21:38 ` Max Reitz
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2018-03-12 19:37 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, den, eblake, qemu-devel
On Fri, Mar 09, 2018 at 10:46:10PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to vhdx, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 37 ++++++++++-
> block/vhdx.c | 174 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 167 insertions(+), 44 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2eba0eef7e..3a65909c47 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3699,6 +3699,41 @@
> '*static': 'bool' } }
>
> ##
> +# @BlockdevVhdxSubformat:
> +#
> +# @dynamic: Growing image file
> +# @fixed: Preallocated fixed-size imge file
s/imge/image
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'BlockdevVhdxSubformat',
> + 'data': [ 'dynamic', 'fixed' ] }
> +
> +##
> +# @BlockdevCreateOptionsVhdx:
> +#
> +# Driver specific image creation options for vhdx.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @log-size Log size in bytes (default: 1 MB)
> +# @block-size Block size in bytes (default: 1 MB)
> +# @subformat vhdx subformat (default: dynamic)
> +# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard,
> +# but default. Do not set to 'off' when using 'qemu-img
> +# convert' with subformat=dynamic.
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsVhdx',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*log-size': 'size',
> + '*block-size': 'size',
> + '*subformat': 'BlockdevVhdxSubformat',
> + '*block-state-zero': 'bool' } }
> +
> +##
> # @BlockdevCreateNotSupported:
> #
> # This is used for all drivers that don't support creating images.
> @@ -3753,7 +3788,7 @@
> 'ssh': 'BlockdevCreateOptionsSsh',
> 'throttle': 'BlockdevCreateNotSupported',
> 'vdi': 'BlockdevCreateOptionsVdi',
> - 'vhdx': 'BlockdevCreateNotSupported',
> + 'vhdx': 'BlockdevCreateOptionsVhdx',
> 'vmdk': 'BlockdevCreateNotSupported',
> 'vpc': 'BlockdevCreateNotSupported',
> 'vvfat': 'BlockdevCreateNotSupported',
> diff --git a/block/vhdx.c b/block/vhdx.c
> index d82350d07c..0ce972381f 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -26,6 +26,9 @@
> #include "block/vhdx.h"
> #include "migration/blocker.h"
> #include "qemu/uuid.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qapi-visit-block-core.h"
>
> /* Options for VHDX creation */
>
> @@ -39,6 +42,8 @@ typedef enum VHDXImageType {
> VHDX_TYPE_DIFFERENCING, /* Currently unsupported */
> } VHDXImageType;
>
> +static QemuOptsList vhdx_create_opts;
> +
> /* Several metadata and region table data entries are identified by
> * guids in a MS-specific GUID format. */
>
> @@ -1792,54 +1797,63 @@ exit:
> * .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------.
> * 1MB
> */
> -static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts,
> - Error **errp)
> +static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
> + Error **errp)
> {
> + BlockdevCreateOptionsVhdx *vhdx_opts;
> + BlockBackend *blk = NULL;
> + BlockDriverState *bs = NULL;
> +
> int ret = 0;
> - uint64_t image_size = (uint64_t) 2 * GiB;
> - uint32_t log_size = 1 * MiB;
> - uint32_t block_size = 0;
> + uint64_t image_size;
> + uint32_t log_size;
> + uint32_t block_size;
> uint64_t signature;
> uint64_t metadata_offset;
> bool use_zero_blocks = false;
>
> gunichar2 *creator = NULL;
> glong creator_items;
> - BlockBackend *blk;
> - char *type = NULL;
> VHDXImageType image_type;
> - Error *local_err = NULL;
>
> - image_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> - BDRV_SECTOR_SIZE);
> - log_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_LOG_SIZE, 0);
> - block_size = qemu_opt_get_size_del(opts, VHDX_BLOCK_OPT_BLOCK_SIZE, 0);
> - type = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
> - use_zero_blocks = qemu_opt_get_bool_del(opts, VHDX_BLOCK_OPT_ZERO, true);
> + assert(opts->driver == BLOCKDEV_DRIVER_VHDX);
> + vhdx_opts = &opts->u.vhdx;
> +
> + /* Validate options and set default values */
> +
> + image_size = vhdx_opts->size;
> + block_size = vhdx_opts->block_size;
> +
> + if (!vhdx_opts->has_log_size) {
> + log_size = DEFAULT_LOG_SIZE;
> + } else {
> + log_size = vhdx_opts->log_size;
> + }
> +
> + if (!vhdx_opts->has_block_state_zero) {
> + use_zero_blocks = true;
> + } else {
> + use_zero_blocks = vhdx_opts->block_state_zero;
> + }
>
> if (image_size > VHDX_MAX_IMAGE_SIZE) {
> error_setg_errno(errp, EINVAL, "Image size too large; max of 64TB");
> - ret = -EINVAL;
> - goto exit;
> + return -EINVAL;
> }
>
> - if (type == NULL) {
> - type = g_strdup("dynamic");
> + if (!vhdx_opts->has_subformat) {
> + vhdx_opts->subformat = BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC;
> }
>
> - if (!strcmp(type, "dynamic")) {
> + switch (vhdx_opts->subformat) {
> + case BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC:
> image_type = VHDX_TYPE_DYNAMIC;
> - } else if (!strcmp(type, "fixed")) {
> + break;
> + case BLOCKDEV_VHDX_SUBFORMAT_FIXED:
> image_type = VHDX_TYPE_FIXED;
> - } else if (!strcmp(type, "differencing")) {
> - error_setg_errno(errp, ENOTSUP,
> - "Differencing files not yet supported");
Just a comment, a minor change will be we no longer recognize that there is
a difference format, but will have a generic error. I think that is fine.
> - ret = -ENOTSUP;
> - goto exit;
> - } else {
> - error_setg(errp, "Invalid subformat '%s'", type);
> - ret = -EINVAL;
> - goto exit;
> + break;
> + default:
> + g_assert_not_reached();
> }
>
> /* These are pretty arbitrary, and mainly designed to keep the BAT
> @@ -1865,21 +1879,17 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts
> block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX :
> block_size;
>
> - ret = bdrv_create_file(filename, opts, &local_err);
> - if (ret < 0) {
> - error_propagate(errp, local_err);
> - goto exit;
> + /* Create BlockBackend to write to the image */
> + bs = bdrv_open_blockdev_ref(vhdx_opts->file, errp);
> + if (bs == NULL) {
> + return -EIO;
> }
>
> - blk = blk_new_open(filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> - &local_err);
> - if (blk == NULL) {
> - error_propagate(errp, local_err);
> - ret = -EIO;
> - goto exit;
> + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
> + ret = blk_insert_bs(blk, bs, errp);
> + if (ret < 0) {
> + goto delete_and_exit;
> }
> -
> blk_set_allow_write_beyond_eof(blk, true);
>
> /* Create (A) */
> @@ -1931,12 +1941,89 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts
>
> delete_and_exit:
> blk_unref(blk);
> -exit:
> - g_free(type);
> + bdrv_unref(bs);
> g_free(creator);
> return ret;
> }
>
> +static int coroutine_fn vhdx_co_create_opts(const char *filename,
> + QemuOpts *opts,
> + Error **errp)
> +{
> + BlockdevCreateOptions *create_options = NULL;
> + QDict *qdict = NULL;
> + QObject *qobj;
> + Visitor *v;
> + BlockDriverState *bs = NULL;
> + Error *local_err = NULL;
> + int ret;
> +
> + static const QDictRenames opt_renames[] = {
> + { VHDX_BLOCK_OPT_LOG_SIZE, "log-size" },
> + { VHDX_BLOCK_OPT_BLOCK_SIZE, "block-size" },
> + { VHDX_BLOCK_OPT_ZERO, "block-state-zero" },
> + { NULL, NULL },
> + };
> +
> + /* Parse options and convert legacy syntax */
> + qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vhdx_create_opts, true);
> +
> + if (!qdict_rename_keys(qdict, opt_renames, errp)) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Create and open the file (protocol layer) */
> + ret = bdrv_create_file(filename, opts, &local_err);
> + if (ret < 0) {
> + error_propagate(errp, local_err);
> + goto fail;
> + }
> +
> + bs = bdrv_open(filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
> + if (bs == NULL) {
> + ret = -EIO;
> + goto fail;
> + }
> +
> + /* Now get the QAPI type BlockdevCreateOptions */
> + qdict_put_str(qdict, "driver", "vhdx");
> + qdict_put_str(qdict, "file", bs->node_name);
> +
> + qobj = qdict_crumple(qdict, errp);
> + QDECREF(qdict);
> + qdict = qobject_to_qdict(qobj);
> + if (qdict == NULL) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
> + visit_free(v);
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Silently round up size */
> + assert(create_options->driver == BLOCKDEV_DRIVER_VHDX);
> + create_options->u.vhdx.size =
> + ROUND_UP(create_options->u.vhdx.size, BDRV_SECTOR_SIZE);
> +
> + /* Create the vhdx image (format layer) */
> + ret = vhdx_co_create(create_options, errp);
> +
> +fail:
> + QDECREF(qdict);
> + bdrv_unref(bs);
> + qapi_free_BlockdevCreateOptions(create_options);
> + return ret;
> +}
> +
> /* If opened r/w, the VHDX driver will automatically replay the log,
> * if one is present, inside the vhdx_open() call.
> *
> @@ -2005,6 +2092,7 @@ static BlockDriver bdrv_vhdx = {
> .bdrv_child_perm = bdrv_format_default_perms,
> .bdrv_co_readv = vhdx_co_readv,
> .bdrv_co_writev = vhdx_co_writev,
> + .bdrv_co_create = vhdx_co_create,
> .bdrv_co_create_opts = vhdx_co_create_opts,
> .bdrv_get_info = vhdx_get_info,
> .bdrv_co_check = vhdx_co_check,
> --
> 2.13.6
>
VHDX image files created look correct, so aside from the minor typo:
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] vhdx: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 6/7] vhdx: " Kevin Wolf
2018-03-12 19:37 ` Jeff Cody
@ 2018-03-12 21:38 ` Max Reitz
1 sibling, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-12 21:38 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5244 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to vhdx, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 37 ++++++++++-
> block/vhdx.c | 174 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 167 insertions(+), 44 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2eba0eef7e..3a65909c47 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3699,6 +3699,41 @@
> '*static': 'bool' } }
>
> ##
> +# @BlockdevVhdxSubformat:
> +#
> +# @dynamic: Growing image file
> +# @fixed: Preallocated fixed-size imge file
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'BlockdevVhdxSubformat',
> + 'data': [ 'dynamic', 'fixed' ] }
> +
> +##
> +# @BlockdevCreateOptionsVhdx:
> +#
> +# Driver specific image creation options for vhdx.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @log-size Log size in bytes (default: 1 MB)
> +# @block-size Block size in bytes (default: 1 MB)
Is it? Currently, the default size is actually 0 and you keep that by a
simple "block_size = vhdx_opts->block_size;" assignment. But the old
help text also states: "0 means auto-calculate based on image size."
This is reflected in the code, even after this patch. 0 can mean 8, 16,
32, or 64 MB.
> +# @subformat vhdx subformat (default: dynamic)
> +# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard,
> +# but default. Do not set to 'off' when using 'qemu-img
> +# convert' with subformat=dynamic.
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsVhdx',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*log-size': 'size',
> + '*block-size': 'size',
> + '*subformat': 'BlockdevVhdxSubformat',
> + '*block-state-zero': 'bool' } }
> +
> +##
> # @BlockdevCreateNotSupported:
> #
> # This is used for all drivers that don't support creating images.
[...]
> index d82350d07c..0ce972381f 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
[...]
> @@ -1792,54 +1797,63 @@ exit:
> * .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------.
> * 1MB
> */
> -static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts,
> - Error **errp)
> +static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
> + Error **errp)
> {
[...]
>
> - if (!strcmp(type, "dynamic")) {
> + switch (vhdx_opts->subformat) {
> + case BLOCKDEV_VHDX_SUBFORMAT_DYNAMIC:
> image_type = VHDX_TYPE_DYNAMIC;
> - } else if (!strcmp(type, "fixed")) {
> + break;
> + case BLOCKDEV_VHDX_SUBFORMAT_FIXED:
> image_type = VHDX_TYPE_FIXED;
> - } else if (!strcmp(type, "differencing")) {
> - error_setg_errno(errp, ENOTSUP,
> - "Differencing files not yet supported");
> - ret = -ENOTSUP;
> - goto exit;
> - } else {
> - error_setg(errp, "Invalid subformat '%s'", type);
> - ret = -EINVAL;
> - goto exit;
> + break;
> + default:
> + g_assert_not_reached();
> }
As a follow-up, it might be reasonable to replace VHDXImageType by
BlockdevVhdxSubformat.
>
> /* These are pretty arbitrary, and mainly designed to keep the BAT
> @@ -1865,21 +1879,17 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename, QemuOpts *opts
There is some code not shown here that silently rounds the log_size and
the block_size to 1 MB, and...
> block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX :
> block_size;
...this. In other drivers you seemed to follow the approach of not
doing this kind of rounding in the blockdev-create path but only in the
legacy interface. Is there a reason for doing it differently here?
Max
>
> - ret = bdrv_create_file(filename, opts, &local_err);
> - if (ret < 0) {
> - error_propagate(errp, local_err);
> - goto exit;
> + /* Create BlockBackend to write to the image */
> + bs = bdrv_open_blockdev_ref(vhdx_opts->file, errp);
> + if (bs == NULL) {
> + return -EIO;
> }
>
> - blk = blk_new_open(filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> - &local_err);
> - if (blk == NULL) {
> - error_propagate(errp, local_err);
> - ret = -EIO;
> - goto exit;
> + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
> + ret = blk_insert_bs(blk, bs, errp);
> + if (ret < 0) {
> + goto delete_and_exit;
> }
> -
> blk_set_allow_write_beyond_eof(blk, true);
>
> /* Create (A) */
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 7/7] vpc: Support .bdrv_co_create
2018-03-09 21:46 [Qemu-devel] [PATCH 0/7] block: .bdrv_co_create for format drivers Kevin Wolf
` (5 preceding siblings ...)
2018-03-09 21:46 ` [Qemu-devel] [PATCH 6/7] vhdx: " Kevin Wolf
@ 2018-03-09 21:46 ` Kevin Wolf
2018-03-12 21:49 ` Max Reitz
6 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2018-03-09 21:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, den, jcody, eblake, qemu-devel
This adds the .bdrv_co_create driver callback to vpc, which
enables image creation over QMP.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 33 ++++++++++-
block/vpc.c | 152 ++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 147 insertions(+), 38 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3a65909c47..ca645a0067 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3734,6 +3734,37 @@
'*block-state-zero': 'bool' } }
##
+# @BlockdevVpcSubformat:
+#
+# @dynamic: Growing image file
+# @fixed: Preallocated fixed-size imge file
+#
+# Since: 2.12
+##
+{ 'enum': 'BlockdevVpcSubformat',
+ 'data': [ 'dynamic', 'fixed' ] }
+
+##
+# @BlockdevCreateOptionsVpc:
+#
+# Driver specific image creation options for vpc (VHD).
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+# @subformat vhdx subformat (default: dynamic)
+# @force-size Force use of the exact byte size instead of rounding to the
+# next size that can be represented in CHS geometry
+# (default: false)
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsVpc',
+ 'data': { 'file': 'BlockdevRef',
+ 'size': 'size',
+ '*subformat': 'BlockdevVpcSubformat',
+ '*force-size': 'bool' } }
+
+##
# @BlockdevCreateNotSupported:
#
# This is used for all drivers that don't support creating images.
@@ -3790,7 +3821,7 @@
'vdi': 'BlockdevCreateOptionsVdi',
'vhdx': 'BlockdevCreateOptionsVhdx',
'vmdk': 'BlockdevCreateNotSupported',
- 'vpc': 'BlockdevCreateNotSupported',
+ 'vpc': 'BlockdevCreateOptionsVpc',
'vvfat': 'BlockdevCreateNotSupported',
'vxhs': 'BlockdevCreateNotSupported'
} }
diff --git a/block/vpc.c b/block/vpc.c
index b2e2b9ebd4..8824211713 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -32,6 +32,9 @@
#include "migration/blocker.h"
#include "qemu/bswap.h"
#include "qemu/uuid.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-block-core.h"
/**************************************************************/
@@ -166,6 +169,8 @@ static QemuOptsList vpc_runtime_opts = {
}
};
+static QemuOptsList vpc_create_opts;
+
static uint32_t vpc_checksum(uint8_t* buf, size_t size)
{
uint32_t res = 0;
@@ -897,12 +902,15 @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
return ret;
}
-static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts,
- Error **errp)
+static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
+ Error **errp)
{
+ BlockdevCreateOptionsVpc *vpc_opts;
+ BlockBackend *blk = NULL;
+ BlockDriverState *bs = NULL;
+
uint8_t buf[1024];
VHDFooter *footer = (VHDFooter *) buf;
- char *disk_type_param;
int i;
uint16_t cyls = 0;
uint8_t heads = 0;
@@ -911,45 +919,38 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts,
int64_t total_size;
int disk_type;
int ret = -EIO;
- bool force_size;
- Error *local_err = NULL;
- BlockBackend *blk = NULL;
- /* Read out options */
- total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE);
- disk_type_param = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
- if (disk_type_param) {
- if (!strcmp(disk_type_param, "dynamic")) {
- disk_type = VHD_DYNAMIC;
- } else if (!strcmp(disk_type_param, "fixed")) {
- disk_type = VHD_FIXED;
- } else {
- error_setg(errp, "Invalid disk type, %s", disk_type_param);
- ret = -EINVAL;
- goto out;
- }
- } else {
+ assert(opts->driver == BLOCKDEV_DRIVER_VPC);
+ vpc_opts = &opts->u.vpc;
+
+ /* Validate options and set default values */
+ total_size = vpc_opts->size;
+
+ if (!vpc_opts->has_subformat) {
+ vpc_opts->subformat = BLOCKDEV_VPC_SUBFORMAT_DYNAMIC;
+ }
+ switch (vpc_opts->subformat) {
+ case BLOCKDEV_VPC_SUBFORMAT_DYNAMIC:
disk_type = VHD_DYNAMIC;
+ break;
+ case BLOCKDEV_VPC_SUBFORMAT_FIXED:
+ disk_type = VHD_FIXED;
+ break;
+ default:
+ g_assert_not_reached();
}
- force_size = qemu_opt_get_bool_del(opts, VPC_OPT_FORCE_SIZE, false);
-
- ret = bdrv_create_file(filename, opts, &local_err);
- if (ret < 0) {
- error_propagate(errp, local_err);
- goto out;
+ /* Create BlockBackend to write to the image */
+ bs = bdrv_open_blockdev_ref(vpc_opts->file, errp);
+ if (bs == NULL) {
+ return -EIO;
}
- blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
- if (blk == NULL) {
- error_propagate(errp, local_err);
- ret = -EIO;
+ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+ ret = blk_insert_bs(blk, bs, errp);
+ if (ret < 0) {
goto out;
}
-
blk_set_allow_write_beyond_eof(blk, true);
/*
@@ -961,7 +962,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts,
* we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use
* the image size from the VHD footer to calculate total_sectors.
*/
- if (force_size) {
+ if (vpc_opts->force_size) {
/* This will force the use of total_size for sector count, below */
cyls = VHD_CHS_MAX_C;
heads = VHD_CHS_MAX_H;
@@ -990,7 +991,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts,
memset(buf, 0, 1024);
memcpy(footer->creator, "conectix", 8);
- if (force_size) {
+ if (vpc_opts->force_size) {
memcpy(footer->creator_app, "qem2", 4);
} else {
memcpy(footer->creator_app, "qemu", 4);
@@ -1032,10 +1033,86 @@ static int coroutine_fn vpc_co_create_opts(const char *filename, QemuOpts *opts,
out:
blk_unref(blk);
- g_free(disk_type_param);
+ bdrv_unref(bs);
+ return ret;
+}
+
+static int coroutine_fn vpc_co_create_opts(const char *filename,
+ QemuOpts *opts, Error **errp)
+{
+ BlockdevCreateOptions *create_options = NULL;
+ QDict *qdict = NULL;
+ QObject *qobj;
+ Visitor *v;
+ BlockDriverState *bs = NULL;
+ Error *local_err = NULL;
+ int ret;
+
+ static const QDictRenames opt_renames[] = {
+ { VPC_OPT_FORCE_SIZE, "force-size" },
+ { NULL, NULL },
+ };
+
+ /* Parse options and convert legacy syntax */
+ qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vpc_create_opts, true);
+
+ if (!qdict_rename_keys(qdict, opt_renames, errp)) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Create and open the file (protocol layer) */
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto fail;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto fail;
+ }
+
+ /* Now get the QAPI type BlockdevCreateOptions */
+ qdict_put_str(qdict, "driver", "vpc");
+ qdict_put_str(qdict, "file", bs->node_name);
+
+ qobj = qdict_crumple(qdict, errp);
+ QDECREF(qdict);
+ qdict = qobject_to_qdict(qobj);
+ if (qdict == NULL) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+ visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+ visit_free(v);
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Silently round up size */
+ assert(create_options->driver == BLOCKDEV_DRIVER_VPC);
+ create_options->u.vpc.size =
+ ROUND_UP(create_options->u.vpc.size, BDRV_SECTOR_SIZE);
+
+ /* Create the vpc image (format layer) */
+ ret = vpc_co_create(create_options, errp);
+
+fail:
+ QDECREF(qdict);
+ bdrv_unref(bs);
+ qapi_free_BlockdevCreateOptions(create_options);
return ret;
}
+
static int vpc_has_zero_init(BlockDriverState *bs)
{
BDRVVPCState *s = bs->opaque;
@@ -1096,6 +1173,7 @@ static BlockDriver bdrv_vpc = {
.bdrv_close = vpc_close,
.bdrv_reopen_prepare = vpc_reopen_prepare,
.bdrv_child_perm = bdrv_format_default_perms,
+ .bdrv_co_create = vpc_co_create,
.bdrv_co_create_opts = vpc_co_create_opts,
.bdrv_co_preadv = vpc_co_preadv,
--
2.13.6
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] vpc: Support .bdrv_co_create
2018-03-09 21:46 ` [Qemu-devel] [PATCH 7/7] vpc: " Kevin Wolf
@ 2018-03-12 21:49 ` Max Reitz
2018-03-13 11:32 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Max Reitz @ 2018-03-12 21:49 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]
On 2018-03-09 22:46, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to vpc, which
> enables image creation over QMP.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 33 ++++++++++-
> block/vpc.c | 152 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 147 insertions(+), 38 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 3a65909c47..ca645a0067 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3734,6 +3734,37 @@
> '*block-state-zero': 'bool' } }
>
> ##
> +# @BlockdevVpcSubformat:
> +#
> +# @dynamic: Growing image file
> +# @fixed: Preallocated fixed-size imge file
s/imge/image/
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'BlockdevVpcSubformat',
> + 'data': [ 'dynamic', 'fixed' ] }
> +
> +##
> +# @BlockdevCreateOptionsVpc:
> +#
> +# Driver specific image creation options for vpc (VHD).
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +# @subformat vhdx subformat (default: dynamic)
> +# @force-size Force use of the exact byte size instead of rounding to the
> +# next size that can be represented in CHS geometry
> +# (default: false)
Now that's weird, again considering your previous approach of only
rounding things in the legacy path and instead throwing errors from
blockdev-create. If you think this is OK to have here, than that's OK
with me, but I'm not sure this is the ideal way.
Alternatives:
1. Swap the default, not sure this is such a good idea either.
2. Maybe add an enum instead. Default: If the given size doesn't fit
CHS, generate an error. Second choice: Use the given size, even if it
doesn't fit. Third choice: Round to CHS.
I don't want to be stuck up, but once it's a public interface...
So if you think the bool is OK like it is...
Reviewed-by: Max Reitz <mreitz@redhat.com>
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsVpc',
> + 'data': { 'file': 'BlockdevRef',
> + 'size': 'size',
> + '*subformat': 'BlockdevVpcSubformat',
> + '*force-size': 'bool' } }
> +
> +##
> # @BlockdevCreateNotSupported:
> #
> # This is used for all drivers that don't support creating images.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] vpc: Support .bdrv_co_create
2018-03-12 21:49 ` Max Reitz
@ 2018-03-13 11:32 ` Kevin Wolf
2018-03-13 12:25 ` Max Reitz
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2018-03-13 11:32 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]
Am 12.03.2018 um 22:49 hat Max Reitz geschrieben:
> On 2018-03-09 22:46, Kevin Wolf wrote:
> > This adds the .bdrv_co_create driver callback to vpc, which
> > enables image creation over QMP.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/block-core.json | 33 ++++++++++-
> > block/vpc.c | 152 ++++++++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 147 insertions(+), 38 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 3a65909c47..ca645a0067 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3734,6 +3734,37 @@
> > '*block-state-zero': 'bool' } }
> >
> > ##
> > +# @BlockdevVpcSubformat:
> > +#
> > +# @dynamic: Growing image file
> > +# @fixed: Preallocated fixed-size imge file
>
> s/imge/image/
>
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'enum': 'BlockdevVpcSubformat',
> > + 'data': [ 'dynamic', 'fixed' ] }
> > +
> > +##
> > +# @BlockdevCreateOptionsVpc:
> > +#
> > +# Driver specific image creation options for vpc (VHD).
> > +#
> > +# @file Node to create the image format on
> > +# @size Size of the virtual disk in bytes
> > +# @subformat vhdx subformat (default: dynamic)
> > +# @force-size Force use of the exact byte size instead of rounding to the
> > +# next size that can be represented in CHS geometry
> > +# (default: false)
>
> Now that's weird, again considering your previous approach of only
> rounding things in the legacy path and instead throwing errors from
> blockdev-create. If you think this is OK to have here, than that's OK
> with me, but I'm not sure this is the ideal way.
Hmm... That's a tough one.
There are a two major differences between VHD and the other image
formats: The first is that rounding is part of the VHD spec. The other
is that while other drivers have reasonable alignment restrictions that
never cause a problem anyway (because people say just '8G' instead of
some odd number), CHS alignment is not reasonable (because '8G' and
similar things will most probably fail).
And then there's the well-known problem that MS is inconsistent with
itself, so force-size=off is required to make images that work with
Virtual PC, but force-size=on may be needed for unaligned image sizes
that HyperV allows, iirc.
> Alternatives:
>
> 1. Swap the default, not sure this is such a good idea either.
>
> 2. Maybe add an enum instead. Default: If the given size doesn't fit
> CHS, generate an error. Second choice: Use the given size, even if it
> doesn't fit. Third choice: Round to CHS.
Maybe we should keep force-size, but make it an error if the size isn't
already aligned (consistent with other block drivers).
The legacy code path could still round, but print a deprecation warning.
Once we get rid of the legacy path, users will have to specify sizes
with correct alignment. The error message could suggest using the
rounded value for Virtual PC compatibility or force-share=on otherwise.
That wouldn't be very nice to use, but maybe it's the best we can make
out of a messed up format like VHD.
> I don't want to be stuck up, but once it's a public interface...
The good thing is that it's still x-blockdev-create.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] vpc: Support .bdrv_co_create
2018-03-13 11:32 ` Kevin Wolf
@ 2018-03-13 12:25 ` Max Reitz
0 siblings, 0 replies; 26+ messages in thread
From: Max Reitz @ 2018-03-13 12:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, den, jcody, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3791 bytes --]
On 2018-03-13 12:32, Kevin Wolf wrote:
> Am 12.03.2018 um 22:49 hat Max Reitz geschrieben:
>> On 2018-03-09 22:46, Kevin Wolf wrote:
>>> This adds the .bdrv_co_create driver callback to vpc, which
>>> enables image creation over QMP.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> qapi/block-core.json | 33 ++++++++++-
>>> block/vpc.c | 152 ++++++++++++++++++++++++++++++++++++++-------------
>>> 2 files changed, 147 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 3a65909c47..ca645a0067 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3734,6 +3734,37 @@
>>> '*block-state-zero': 'bool' } }
>>>
>>> ##
>>> +# @BlockdevVpcSubformat:
>>> +#
>>> +# @dynamic: Growing image file
>>> +# @fixed: Preallocated fixed-size imge file
>>
>> s/imge/image/
>>
>>> +#
>>> +# Since: 2.12
>>> +##
>>> +{ 'enum': 'BlockdevVpcSubformat',
>>> + 'data': [ 'dynamic', 'fixed' ] }
>>> +
>>> +##
>>> +# @BlockdevCreateOptionsVpc:
>>> +#
>>> +# Driver specific image creation options for vpc (VHD).
>>> +#
>>> +# @file Node to create the image format on
>>> +# @size Size of the virtual disk in bytes
>>> +# @subformat vhdx subformat (default: dynamic)
>>> +# @force-size Force use of the exact byte size instead of rounding to the
>>> +# next size that can be represented in CHS geometry
>>> +# (default: false)
>>
>> Now that's weird, again considering your previous approach of only
>> rounding things in the legacy path and instead throwing errors from
>> blockdev-create. If you think this is OK to have here, than that's OK
>> with me, but I'm not sure this is the ideal way.
>
> Hmm... That's a tough one.
>
> There are a two major differences between VHD and the other image
> formats: The first is that rounding is part of the VHD spec. The other
> is that while other drivers have reasonable alignment restrictions that
> never cause a problem anyway (because people say just '8G' instead of
> some odd number), CHS alignment is not reasonable (because '8G' and
> similar things will most probably fail).
Well, if it's part of the spec, then I'll be OK with keeping the flag as
it is.
> And then there's the well-known problem that MS is inconsistent with
> itself, so force-size=off is required to make images that work with
> Virtual PC, but force-size=on may be needed for unaligned image sizes
> that HyperV allows, iirc.
>
>> Alternatives:
>>
>> 1. Swap the default, not sure this is such a good idea either.
>>
>> 2. Maybe add an enum instead. Default: If the given size doesn't fit
>> CHS, generate an error. Second choice: Use the given size, even if it
>> doesn't fit. Third choice: Round to CHS.
>
> Maybe we should keep force-size, but make it an error if the size isn't
> already aligned (consistent with other block drivers).
>
> The legacy code path could still round, but print a deprecation warning.
> Once we get rid of the legacy path, users will have to specify sizes
> with correct alignment. The error message could suggest using the
> rounded value for Virtual PC compatibility or force-share=on otherwise.
>
> That wouldn't be very nice to use, but maybe it's the best we can make
> out of a messed up format like VHD.
Sounds reasonable to me, although this would probably just result in
management tools copying qemu's code (or maybe it's code directly from
the spec?) to do the rounding so that qemu shuts up.
>> I don't want to be stuck up, but once it's a public interface...
>
> The good thing is that it's still x-blockdev-create.
Ah, right.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread