block: .bdrv_co_create for format drivers

This adds the .bdrv_co_create driver callback to parallels, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  18 ++++-
 block/parallels.c    | 199 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 168 insertions(+), 49 deletions(-)

block/parallels.c +0 -0

index c13cb619e6..2da5e56a9d 100644

block/parallels.c +0 -0

@@ -542,24 +580,107 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,

block/parallels.c +4 -0

+static int coroutine_fn parallels_co_create_opts(const char *filename,
+                                                 QemuOpts *opts,
+                                                 Error **errp)
+{

block/parallels.c +10 -0

+    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);

This adds the .bdrv_co_create driver callback to parallels, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  18 ++++-
 block/parallels.c    | 199 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 168 insertions(+), 49 deletions(-)

qapi/block-core.json +17 -1

index 07039bfe9c..d38058eeab 100644
@@ -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',

block/parallels.c +151 -48

index c13cb619e6..2da5e56a9d 100644
@@ -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, &parallels_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    = &parallels_create_opts,

Signature · 1 line

2.13.6

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 <[email protected]>

---
 tests/qemu-iotests/181   | 2 +-
 tests/qemu-iotests/check | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

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 <[email protected]>

---
 tests/qemu-iotests/181   | 2 +-
 tests/qemu-iotests/check | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

tests/qemu-iotests/181 +1 -1

index 0c91e8f9de..5e767c6195 100755
@@ -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
 

tests/qemu-iotests/check +0 -1

index e6b6ff7a04..469142cd58 100755
@@ -284,7 +284,6 @@ testlist options
 
         -parallels)
             IMGFMT=parallels
-            IMGFMT_GENERIC=false
             xpand=false
             ;;
 

Signature · 1 line

2.13.6

This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
  qapi/block-core.json |  21 +++++-
  block/qcow.c         | 196 ++++++++++++++++++++++++++++++++++-----------------
  2 files changed, 150 insertions(+), 67 deletions(-)

Quoted 8 lines

This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
  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?

This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  21 +++++-
 block/qcow.c         | 196 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 150 insertions(+), 67 deletions(-)

This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  21 +++++-
 block/qcow.c         | 196 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 150 insertions(+), 67 deletions(-)

qapi/block-core.json +20 -1

index d38058eeab..c81677c434 100644
@@ -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',

block/qcow.c +130 -66

index 47a18d9a3a..2e3770ca63 100644
@@ -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,

Signature · 1 line

2.13.6

This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
  qapi/block-core.json |  21 +++++-
  block/qcow.c         | 196 ++++++++++++++++++++++++++++++++++-----------------
  2 files changed, 150 insertions(+), 67 deletions(-)

##

Quoted 8 lines · Kevin Wolf

This adds the .bdrv_co_create driver callback to qcow, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
  qapi/block-core.json |  21 +++++-
  block/qcow.c         | 196 ++++++++++++++++++++++++++++++++++-----------------
  2 files changed, 150 insertions(+), 67 deletions(-)

qapi/block-core.json line 3508

Quoted 10 lines

##

qapi/block-core.json +9 -0

+# @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.

This adds the .bdrv_co_create driver callback to qed, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
  qapi/block-core.json |  25 ++++++-
  block/qed.c          | 204 ++++++++++++++++++++++++++++++++++-----------------
  2 files changed, 162 insertions(+), 67 deletions(-)

This adds the .bdrv_co_create driver callback to qed, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  25 ++++++-
 block/qed.c          | 204 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 162 insertions(+), 67 deletions(-)

This adds the .bdrv_co_create driver callback to vdi, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  21 ++++++-
 block/vdi.c          | 169 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 148 insertions(+), 42 deletions(-)

This adds the .bdrv_co_create driver callback to vhdx, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  37 ++++++++++-
 block/vhdx.c         | 174 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 167 insertions(+), 44 deletions(-)

qapi/block-core.json +0 -0

index 2eba0eef7e..3a65909c47 100644

qapi/block-core.json +32 -1

+#
+# 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',

block/vhdx.c +36 -19

index d82350d07c..0ce972381f 100644
@@ -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")) {

block/vhdx.c +91 -20

-        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,

Signature · 1 line

2.13.6

This adds the .bdrv_co_create driver callback to vhdx, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  37 ++++++++++-
 block/vhdx.c         | 174 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 167 insertions(+), 44 deletions(-)

qapi/block-core.json +9 -0

index 2eba0eef7e..3a65909c47 100644
@@ -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' ] }

qapi/block-core.json +16 -0

+# @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

block/vhdx.c +0 -0

block/vhdx.c +2 -2

@@ -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)
 {

block/vhdx.c +4 -5

-    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");

block/vhdx.c +8 -12

-    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) */

This adds the .bdrv_co_create driver callback to vpc, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  33 ++++++++++-
 block/vpc.c          | 152 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 147 insertions(+), 38 deletions(-)

qapi/block-core.json +0 -0

index 3a65909c47..ca645a0067 100644

qapi/block-core.json +7 -0

+#
+# Since: 2.12
+##
+{ 'enum': 'BlockdevVpcSubformat',
+  'data': [ 'dynamic', 'fixed' ] }
+
+##

qapi/block-core.json +10 -0

+#
+# 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.

Quoted 14 lines

This adds the .bdrv_co_create driver callback to vpc, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  33 ++++++++++-
 block/vpc.c          | 152 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 147 insertions(+), 38 deletions(-)

qapi/block-core.json +0 -0

index 3a65909c47..ca645a0067 100644

qapi/block-core.json line 3740

Quoted 7 lines

qapi/block-core.json +4 -0

@@ -3734,6 +3734,37 @@
             '*block-state-zero':    'bool' } }
 
 ##
+# @BlockdevVpcSubformat:
+#
+# @dynamic: Growing image file
+# @fixed:   Preallocated fixed-size imge file

s/imge/image/

Quoted 14 lines

qapi/block-core.json +14 -0

+#
+# 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)

qapi/block-core.json line 3757

Quoted 3 lines

qapi/block-core.json +3 -0

+# @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...

Am 12.03.2018 um 22:49 hat Max Reitz geschrieben:

Quoted 39 lines · Kevin Wolf

Quoted 14 lines

This adds the .bdrv_co_create driver callback to vpc, which enables image creation over QMP.

Signed-off-by Kevin Wolf <[email protected]>

---
 qapi/block-core.json |  33 ++++++++++-
 block/vpc.c          | 152 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 147 insertions(+), 38 deletions(-)

qapi/block-core.json +0 -0

index 3a65909c47..ca645a0067 100644

qapi/block-core.json line 3740

Quoted 7 lines

qapi/block-core.json +4 -0

@@ -3734,6 +3734,37 @@
             '*block-state-zero':    'bool' } }
 
 ##
+# @BlockdevVpcSubformat:
+#
+# @dynamic: Growing image file
+# @fixed:   Preallocated fixed-size imge file

s/imge/image/

Quoted 14 lines

qapi/block-core.json +14 -0

+#
+# 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)

Quoted 8 lines

qapi/block-core.json line 3757

Quoted 3 lines

qapi/block-core.json +3 -0

+# @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.

Quoted 7 lines

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.

Quoted 1 line

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.