[PATCH 0/2] Fix the generic image creation code

[PATCH 0/2] Fix the generic image creation code

[Maxim Levitsky]

The recent patches from Max Reitz allowed some block drivers to not
provide the .bdrv_co_create_opts and still allow qemu-img to
create/format images as long as the image is already existing
(that is the case with various block storage drivers like nbd/iscsi/nvme, etc)

However it was found out that some places in the code depend on the
.bdrv_co_create_opts/.create_opts to be != NULL to decide if to allow
image creation.

To avoid adding failback code to all these places, just make generic failback
code be used by the drivers that need it, so that for outside user, there
is no diffirence if failback was used or not.

Best regards,
	Maxim Levitsky

Maxim Levitsky (2):
  block: pass BlockDriver reference to the .bdrv_co_create
  block: trickle down the fallback image creation function use to the
    block drivers

 block.c                   | 38 ++++++++++++++++++++++----------------
 block/crypto.c            |  3 ++-
 block/file-posix.c        | 11 +++++++++--
 block/file-win32.c        |  4 +++-
 block/gluster.c           |  3 ++-
 block/iscsi.c             | 16 ++++------------
 block/nbd.c               |  6 ++++++
 block/nfs.c               |  4 +++-
 block/nvme.c              |  3 +++
 block/parallels.c         |  3 ++-
 block/qcow.c              |  3 ++-
 block/qcow2.c             |  4 +++-
 block/qed.c               |  3 ++-
 block/raw-format.c        |  4 +++-
 block/rbd.c               |  3 ++-
 block/sheepdog.c          |  4 +++-
 block/ssh.c               |  4 +++-
 block/vdi.c               |  4 +++-
 block/vhdx.c              |  3 ++-
 block/vmdk.c              |  4 +++-
 block/vpc.c               |  6 ++++--
 include/block/block.h     |  7 +++++++
 include/block/block_int.h |  3 ++-
 23 files changed, 95 insertions(+), 48 deletions(-)

-- 
2.17.2

[PATCH 1/2] block: pass BlockDriver reference to the .bdrv_co_create

[Maxim Levitsky]

This will allow to reuse a single generic .bdrv_co_create
implementation for several drivers.
No functional changes.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block.c                   | 3 ++-
 block/crypto.c            | 3 ++-
 block/file-posix.c        | 4 +++-
 block/file-win32.c        | 4 +++-
 block/gluster.c           | 3 ++-
 block/nfs.c               | 4 +++-
 block/parallels.c         | 3 ++-
 block/qcow.c              | 3 ++-
 block/qcow2.c             | 4 +++-
 block/qed.c               | 3 ++-
 block/raw-format.c        | 4 +++-
 block/rbd.c               | 3 ++-
 block/sheepdog.c          | 4 +++-
 block/ssh.c               | 4 +++-
 block/vdi.c               | 4 +++-
 block/vhdx.c              | 3 ++-
 block/vmdk.c              | 4 +++-
 block/vpc.c               | 6 ++++--
 include/block/block_int.h | 3 ++-
 19 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index cccae5add9..ff23e20443 100644
--- a/block.c
+++ b/block.c
@@ -483,7 +483,8 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
     CreateCo *cco = opaque;
     assert(cco->drv);
 
-    ret = cco->drv->bdrv_co_create_opts(cco->filename, cco->opts, &local_err);
+    ret = cco->drv->bdrv_co_create_opts(cco->drv,
+                                        cco->filename, cco->opts, &local_err);
     error_propagate(&cco->err, local_err);
     cco->ret = ret;
 }
diff --git a/block/crypto.c b/block/crypto.c
index 4425ebeb47..d577f89659 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -601,7 +601,8 @@ fail:
     return ret;
 }
 
-static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
+static int coroutine_fn block_crypto_co_create_opts_luks(BlockDriver *drv,
+                                                         const char *filename,
                                                          QemuOpts *opts,
                                                          Error **errp)
 {
diff --git a/block/file-posix.c b/block/file-posix.c
index 9bc3838b2a..65bc980bc4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2405,7 +2405,9 @@ out:
     return result;
 }
 
-static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
                                            Error **errp)
 {
     BlockdevCreateOptions options;
diff --git a/block/file-win32.c b/block/file-win32.c
index 77e8ff7b68..15859839a1 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -588,7 +588,9 @@ static int raw_co_create(BlockdevCreateOptions *options, Error **errp)
     return 0;
 }
 
-static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
                                            Error **errp)
 {
     BlockdevCreateOptions options;
diff --git a/block/gluster.c b/block/gluster.c
index 4fa4a77a47..0aa1f2cda4 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1130,7 +1130,8 @@ out:
     return ret;
 }
 
-static int coroutine_fn qemu_gluster_co_create_opts(const char *filename,
+static int coroutine_fn qemu_gluster_co_create_opts(BlockDriver *drv,
+                                                    const char *filename,
                                                     QemuOpts *opts,
                                                     Error **errp)
 {
diff --git a/block/nfs.c b/block/nfs.c
index 9a6311e270..cc2413d5ab 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -662,7 +662,9 @@ out:
     return ret;
 }
 
-static int coroutine_fn nfs_file_co_create_opts(const char *url, QemuOpts *opts,
+static int coroutine_fn nfs_file_co_create_opts(BlockDriver *drv,
+                                                const char *url,
+                                                QemuOpts *opts,
                                                 Error **errp)
 {
     BlockdevCreateOptions *create_options;
diff --git a/block/parallels.c b/block/parallels.c
index 7a01997659..6d4ed77f16 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -609,7 +609,8 @@ exit:
     goto out;
 }
 
-static int coroutine_fn parallels_co_create_opts(const char *filename,
+static int coroutine_fn parallels_co_create_opts(BlockDriver *drv,
+                                                 const char *filename,
                                                  QemuOpts *opts,
                                                  Error **errp)
 {
diff --git a/block/qcow.c b/block/qcow.c
index fce8989868..8973e4e565 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -934,7 +934,8 @@ exit:
     return ret;
 }
 
-static int coroutine_fn qcow_co_create_opts(const char *filename,
+static int coroutine_fn qcow_co_create_opts(BlockDriver *drv,
+                                            const char *filename,
                                             QemuOpts *opts, Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
diff --git a/block/qcow2.c b/block/qcow2.c
index d1da3d91db..5f65fce924 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3558,7 +3558,9 @@ out:
     return ret;
 }
 
-static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
+                                             const char *filename,
+                                             QemuOpts *opts,
                                              Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
diff --git a/block/qed.c b/block/qed.c
index d8c4e5fb1e..1af9b3cb1d 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -720,7 +720,8 @@ out:
     return ret;
 }
 
-static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
+static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver *drv,
+                                                const char *filename,
                                                 QemuOpts *opts,
                                                 Error **errp)
 {
diff --git a/block/raw-format.c b/block/raw-format.c
index 3a76ec7dd2..93b25e1b6b 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -419,7 +419,9 @@ static int raw_has_zero_init_truncate(BlockDriverState *bs)
     return bdrv_has_zero_init_truncate(bs->file->bs);
 }
 
-static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
                                            Error **errp)
 {
     return bdrv_create_file(filename, opts, errp);
diff --git a/block/rbd.c b/block/rbd.c
index 84115d34b4..e637639a07 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -437,7 +437,8 @@ static int qemu_rbd_co_create(BlockdevCreateOptions *options, Error **errp)
     return qemu_rbd_do_create(options, NULL, NULL, errp);
 }
 
-static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
+static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver *drv,
+                                                const char *filename,
                                                 QemuOpts *opts,
                                                 Error **errp)
 {
diff --git a/block/sheepdog.c b/block/sheepdog.c
index cfa84338a2..a8a7e32a41 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2157,7 +2157,9 @@ out:
     return ret;
 }
 
-static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn sd_co_create_opts(BlockDriver *drv,
+                                          const char *filename,
+                                          QemuOpts *opts,
                                           Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
diff --git a/block/ssh.c b/block/ssh.c
index b4375cf7d2..84e92821c0 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -963,7 +963,9 @@ fail:
     return ret;
 }
 
-static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn ssh_co_create_opts(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
                                            Error **errp)
 {
     BlockdevCreateOptions *create_options;
diff --git a/block/vdi.c b/block/vdi.c
index 0142da7233..e1a11f2aa0 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -896,7 +896,9 @@ static int coroutine_fn vdi_co_create(BlockdevCreateOptions *create_options,
     return vdi_co_do_create(create_options, DEFAULT_CLUSTER_SIZE, errp);
 }
 
-static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn vdi_co_create_opts(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
                                            Error **errp)
 {
     QDict *qdict = NULL;
diff --git a/block/vhdx.c b/block/vhdx.c
index f02d2611be..33e57cd656 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2046,7 +2046,8 @@ delete_and_exit:
     return ret;
 }
 
-static int coroutine_fn vhdx_co_create_opts(const char *filename,
+static int coroutine_fn vhdx_co_create_opts(BlockDriver *drv,
+                                            const char *filename,
                                             QemuOpts *opts,
                                             Error **errp)
 {
diff --git a/block/vmdk.c b/block/vmdk.c
index 8466051bc9..218d9c9800 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2588,7 +2588,9 @@ exit:
     return blk;
 }
 
-static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
+static int coroutine_fn vmdk_co_create_opts(BlockDriver *drv,
+                                            const char *filename,
+                                            QemuOpts *opts,
                                             Error **errp)
 {
     Error *local_err = NULL;
diff --git a/block/vpc.c b/block/vpc.c
index a65550298e..6df75e22dc 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1089,8 +1089,10 @@ out:
     return ret;
 }
 
-static int coroutine_fn vpc_co_create_opts(const char *filename,
-                                           QemuOpts *opts, Error **errp)
+static int coroutine_fn vpc_co_create_opts(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
+                                           Error **errp)
 {
     BlockdevCreateOptions *create_options = NULL;
     QDict *qdict;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ae9c4da4d0..57c8ea24b2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -135,7 +135,8 @@ struct BlockDriver {
     void (*bdrv_close)(BlockDriverState *bs);
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
                                        Error **errp);
-    int coroutine_fn (*bdrv_co_create_opts)(const char *filename,
+    int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv,
+                                            const char *filename,
                                             QemuOpts *opts,
                                             Error **errp);
     int (*bdrv_make_empty)(BlockDriverState *bs);
-- 
2.17.2

[PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Maxim Levitsky]

Instead of checking the .bdrv_co_create_opts to see if we need the failback,
just implement the .bdrv_co_create_opts in the drivers that need it.

This way we don't break various places that need to know if the underlying
protocol/format really supports image creation, and this way we still
allow some drivers to not support image creation.

Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007

Note that technically this driver reverts the image creation failback
for the vxhs driver since I don't have a means to test it,
and IMHO it is better to leave it not supported as it was prior to
generic image creation patches.

Also drop iscsi_create_opts which was left accidently

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block.c               | 35 ++++++++++++++++++++---------------
 block/file-posix.c    |  7 ++++++-
 block/iscsi.c         | 16 ++++------------
 block/nbd.c           |  6 ++++++
 block/nvme.c          |  3 +++
 include/block/block.h |  7 +++++++
 6 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index ff23e20443..72fdf56af7 100644
--- a/block.c
+++ b/block.c
@@ -598,8 +598,15 @@ static int create_file_fallback_zero_first_sector(BlockBackend *blk,
     return 0;
 }
 
-static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
-                                     QemuOpts *opts, Error **errp)
+/**
+ * Simple implementation of bdrv_co_create_opts for protocol drivers
+ * which only support creation via opening a file
+ * (usually existing raw storage device)
+ */
+int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
+                                           Error **errp)
 {
     BlockBackend *blk;
     QDict *options;
@@ -663,11 +670,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
         return -ENOENT;
     }
 
-    if (drv->bdrv_co_create_opts) {
-        return bdrv_create(drv, filename, opts, errp);
-    } else {
-        return bdrv_create_file_fallback(filename, drv, opts, errp);
-    }
+    return bdrv_create(drv, filename, opts, errp);
 }
 
 int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
@@ -1592,9 +1595,9 @@ QemuOptsList bdrv_runtime_opts = {
     },
 };
 
-static QemuOptsList fallback_create_opts = {
-    .name = "fallback-create-opts",
-    .head = QTAILQ_HEAD_INITIALIZER(fallback_create_opts.head),
+QemuOptsList bdrv_create_opts_simple = {
+    .name = "simple-create-opts",
+    .head = QTAILQ_HEAD_INITIALIZER(bdrv_create_opts_simple.head),
     .desc = {
         {
             .name = BLOCK_OPT_SIZE,
@@ -5963,13 +5966,15 @@ void bdrv_img_create(const char *filename, const char *fmt,
         return;
     }
 
+    if (!proto_drv->create_opts) {
+        error_setg(errp, "Protocol driver '%s' does not support image creation",
+                   proto_drv->format_name);
+        return;
+    }
+
     /* Create parameter list */
     create_opts = qemu_opts_append(create_opts, drv->create_opts);
-    if (proto_drv->create_opts) {
-        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
-    } else {
-        create_opts = qemu_opts_append(create_opts, &fallback_create_opts);
-    }
+    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
 
diff --git a/block/file-posix.c b/block/file-posix.c
index 65bc980bc4..7e19bbff5f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
+    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
+    .create_opts         = &bdrv_create_opts_simple,
     .mutable_opts        = mutable_opts,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
     .bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
@@ -3639,10 +3641,11 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
+    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
+    .create_opts         = &bdrv_create_opts_simple,
     .mutable_opts        = mutable_opts,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
 
-
     .bdrv_co_preadv         = raw_co_preadv,
     .bdrv_co_pwritev        = raw_co_pwritev,
     .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
@@ -3771,6 +3774,8 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
+    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
+    .create_opts         = &bdrv_create_opts_simple,
     .mutable_opts       = mutable_opts,
 
     .bdrv_co_preadv         = raw_co_preadv,
diff --git a/block/iscsi.c b/block/iscsi.c
index 682abd8e09..14680a436a 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2399,18 +2399,6 @@ out_unlock:
     return r;
 }
 
-static QemuOptsList iscsi_create_opts = {
-    .name = "iscsi-create-opts",
-    .head = QTAILQ_HEAD_INITIALIZER(iscsi_create_opts.head),
-    .desc = {
-        {
-            .name = BLOCK_OPT_SIZE,
-            .type = QEMU_OPT_SIZE,
-            .help = "Virtual disk size"
-        },
-        { /* end of list */ }
-    }
-};
 
 static const char *const iscsi_strong_runtime_opts[] = {
     "transport",
@@ -2434,6 +2422,8 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_parse_filename    = iscsi_parse_filename,
     .bdrv_file_open         = iscsi_open,
     .bdrv_close             = iscsi_close,
+    .bdrv_co_create_opts    = bdrv_co_create_opts_simple,
+    .create_opts            = &bdrv_create_opts_simple,
     .bdrv_reopen_prepare    = iscsi_reopen_prepare,
     .bdrv_reopen_commit     = iscsi_reopen_commit,
     .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
@@ -2471,6 +2461,8 @@ static BlockDriver bdrv_iser = {
     .bdrv_parse_filename    = iscsi_parse_filename,
     .bdrv_file_open         = iscsi_open,
     .bdrv_close             = iscsi_close,
+    .bdrv_co_create_opts    = bdrv_co_create_opts_simple,
+    .create_opts            = &bdrv_create_opts_simple,
     .bdrv_reopen_prepare    = iscsi_reopen_prepare,
     .bdrv_reopen_commit     = iscsi_reopen_commit,
     .bdrv_co_invalidate_cache  = iscsi_co_invalidate_cache,
diff --git a/block/nbd.c b/block/nbd.c
index 976be76647..2160859f64 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -2038,6 +2038,8 @@ static BlockDriver bdrv_nbd = {
     .protocol_name              = "nbd",
     .instance_size              = sizeof(BDRVNBDState),
     .bdrv_parse_filename        = nbd_parse_filename,
+    .bdrv_co_create_opts        = bdrv_co_create_opts_simple,
+    .create_opts                = &bdrv_create_opts_simple,
     .bdrv_file_open             = nbd_open,
     .bdrv_reopen_prepare        = nbd_client_reopen_prepare,
     .bdrv_co_preadv             = nbd_client_co_preadv,
@@ -2063,6 +2065,8 @@ static BlockDriver bdrv_nbd_tcp = {
     .protocol_name              = "nbd+tcp",
     .instance_size              = sizeof(BDRVNBDState),
     .bdrv_parse_filename        = nbd_parse_filename,
+    .bdrv_co_create_opts        = bdrv_co_create_opts_simple,
+    .create_opts                = &bdrv_create_opts_simple,
     .bdrv_file_open             = nbd_open,
     .bdrv_reopen_prepare        = nbd_client_reopen_prepare,
     .bdrv_co_preadv             = nbd_client_co_preadv,
@@ -2088,6 +2092,8 @@ static BlockDriver bdrv_nbd_unix = {
     .protocol_name              = "nbd+unix",
     .instance_size              = sizeof(BDRVNBDState),
     .bdrv_parse_filename        = nbd_parse_filename,
+    .bdrv_co_create_opts        = bdrv_co_create_opts_simple,
+    .create_opts                = &bdrv_create_opts_simple,
     .bdrv_file_open             = nbd_open,
     .bdrv_reopen_prepare        = nbd_client_reopen_prepare,
     .bdrv_co_preadv             = nbd_client_co_preadv,
diff --git a/block/nvme.c b/block/nvme.c
index d41c4bda6e..7b7c0cc5d6 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1333,6 +1333,9 @@ static BlockDriver bdrv_nvme = {
     .protocol_name            = "nvme",
     .instance_size            = sizeof(BDRVNVMeState),
 
+    .bdrv_co_create_opts      = bdrv_co_create_opts_simple,
+    .create_opts              = &bdrv_create_opts_simple,
+
     .bdrv_parse_filename      = nvme_parse_filename,
     .bdrv_file_open           = nvme_file_open,
     .bdrv_close               = nvme_close,
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..7d36ec5433 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -283,6 +283,13 @@ BlockDriver *bdrv_find_format(const char *format_name);
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp);
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
+
+int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
+                                           const char *filename,
+                                           QemuOpts *opts,
+                                           Error **errp);
+extern QemuOptsList bdrv_create_opts_simple;
+
 BlockDriverState *bdrv_new(void);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                  Error **errp);
-- 
2.17.2

Re: [PATCH 0/2] Fix the generic image creation code

[Denis V. Lunev]

On 3/26/20 4:12 AM, Maxim Levitsky wrote:
> The recent patches from Max Reitz allowed some block drivers to not
> provide the .bdrv_co_create_opts and still allow qemu-img to
> create/format images as long as the image is already existing
> (that is the case with various block storage drivers like nbd/iscsi/nvme, etc)
>
> However it was found out that some places in the code depend on the
> .bdrv_co_create_opts/.create_opts to be != NULL to decide if to allow
> image creation.
>
> To avoid adding failback code to all these places, just make generic failback
> code be used by the drivers that need it, so that for outside user, there
> is no diffirence if failback was used or not.
>
> Best regards,
> 	Maxim Levitsky
>
> Maxim Levitsky (2):
>   block: pass BlockDriver reference to the .bdrv_co_create
>   block: trickle down the fallback image creation function use to the
>     block drivers
>
>  block.c                   | 38 ++++++++++++++++++++++----------------
>  block/crypto.c            |  3 ++-
>  block/file-posix.c        | 11 +++++++++--
>  block/file-win32.c        |  4 +++-
>  block/gluster.c           |  3 ++-
>  block/iscsi.c             | 16 ++++------------
>  block/nbd.c               |  6 ++++++
>  block/nfs.c               |  4 +++-
>  block/nvme.c              |  3 +++
>  block/parallels.c         |  3 ++-
>  block/qcow.c              |  3 ++-
>  block/qcow2.c             |  4 +++-
>  block/qed.c               |  3 ++-
>  block/raw-format.c        |  4 +++-
>  block/rbd.c               |  3 ++-
>  block/sheepdog.c          |  4 +++-
>  block/ssh.c               |  4 +++-
>  block/vdi.c               |  4 +++-
>  block/vhdx.c              |  3 ++-
>  block/vmdk.c              |  4 +++-
>  block/vpc.c               |  6 ++++--
>  include/block/block.h     |  7 +++++++
>  include/block/block_int.h |  3 ++-
>  23 files changed, 95 insertions(+), 48 deletions(-)
>
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4581 bytes --]

On 26.03.20 02:12, Maxim Levitsky wrote:
> Instead of checking the .bdrv_co_create_opts to see if we need the failback,
> just implement the .bdrv_co_create_opts in the drivers that need it.
> 
> This way we don't break various places that need to know if the underlying
> protocol/format really supports image creation, and this way we still
> allow some drivers to not support image creation.
> 
> Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007

Thanks, the series looks good to me, just some thoughts below.

> Note that technically this driver reverts the image creation failback
> for the vxhs driver since I don't have a means to test it,
> and IMHO it is better to leave it not supported as it was prior to
> generic image creation patches.

There’s also file-win32.  I don’t really have the means to test that
either, though, so I suppose it’s reasonable to wait until someone
requests it.  OTOH, it shouldn’t be different from file-posix, so maybe
it wouldn’t hurt to support it, too.

We could also take this series for 5.0 as-is, and queue a file-win32
patch for 5.1.

What do you think?

> Also drop iscsi_create_opts which was left accidently
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block.c               | 35 ++++++++++++++++++++---------------
>  block/file-posix.c    |  7 ++++++-
>  block/iscsi.c         | 16 ++++------------
>  block/nbd.c           |  6 ++++++
>  block/nvme.c          |  3 +++
>  include/block/block.h |  7 +++++++
>  6 files changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ff23e20443..72fdf56af7 100644
> --- a/block.c
> +++ b/block.c
> @@ -598,8 +598,15 @@ static int create_file_fallback_zero_first_sector(BlockBackend *blk,
>      return 0;
>  }
>  
> -static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
> -                                     QemuOpts *opts, Error **errp)
> +/**
> + * Simple implementation of bdrv_co_create_opts for protocol drivers
> + * which only support creation via opening a file
> + * (usually existing raw storage device)
> + */
> +int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
> +                                           const char *filename,
> +                                           QemuOpts *opts,
> +                                           Error **errp)

The alignment’s off (in the header, too), but that can be fixed when
this series is applied.

>  {
>      BlockBackend *blk;
>      QDict *options;
> @@ -663,11 +670,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>          return -ENOENT;
>      }
>  
> -    if (drv->bdrv_co_create_opts) {
> -        return bdrv_create(drv, filename, opts, errp);
> -    } else {
> -        return bdrv_create_file_fallback(filename, drv, opts, errp);
> -    }
> +    return bdrv_create(drv, filename, opts, errp);

I thought we’d just let the drivers set BlockDriver.create_opts to
&bdrv_create_opts_simple and keep this bit of code (maybe with an
“else if (drv->create_opts != NULL)” and an
“assert(drv->create_opts == &bdrv_create_opts_simple)”).  That would
make the first patch unnecessary.

OTOH, it’s completely reasonable to pass the object as the first
argument to a class method, so why not.  (Well, technically the
BlockDriver kind of is the class, and the BDS is the object, it’s
weird.)  And it definitely follows what we do elsewhere (to provide
default implementations for block drivers to use).

>  }
>  
>  int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)

[...]

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 65bc980bc4..7e19bbff5f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c

[...]

> @@ -3639,10 +3641,11 @@ static BlockDriver bdrv_host_cdrom = {
>      .bdrv_reopen_prepare = raw_reopen_prepare,
>      .bdrv_reopen_commit  = raw_reopen_commit,
>      .bdrv_reopen_abort   = raw_reopen_abort,
> +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
> +    .create_opts         = &bdrv_create_opts_simple,
>      .mutable_opts        = mutable_opts,
>      .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>  
> -

This line removal seems unrelated, but why not.

Max

>      .bdrv_co_preadv         = raw_co_preadv,
>      .bdrv_co_pwritev        = raw_co_pwritev,
>      .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] Fix the generic image creation code

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1144 bytes --]

On 26.03.20 02:12, Maxim Levitsky wrote:
> The recent patches from Max Reitz allowed some block drivers to not
> provide the .bdrv_co_create_opts and still allow qemu-img to
> create/format images as long as the image is already existing
> (that is the case with various block storage drivers like nbd/iscsi/nvme, etc)
> 
> However it was found out that some places in the code depend on the
> .bdrv_co_create_opts/.create_opts to be != NULL to decide if to allow
> image creation.
> 
> To avoid adding failback code to all these places, just make generic failback
> code be used by the drivers that need it, so that for outside user, there
> is no diffirence if failback was used or not.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (2):
>   block: pass BlockDriver reference to the .bdrv_co_create
>   block: trickle down the fallback image creation function use to the
>     block drivers

Thanks, fixed the function parameter alignment, moved the declarations
from block.h into block_int.h, and applied the series to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] block: pass BlockDriver reference to the .bdrv_co_create

[Eric Blake]

On 3/25/20 8:12 PM, Maxim Levitsky wrote:
> This will allow to reuse a single generic .bdrv_co_create

"allow to ${verb}" is not idiomatic, better is "allow ${subject} to 
${verb}" or "allow ${verb}ing".  In this case, I'd go with:

This will allow the reuse of a single...

> implementation for several drivers.
> No functional changes.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Eric Blake]

On 3/25/20 8:12 PM, Maxim Levitsky wrote:
> Instead of checking the .bdrv_co_create_opts to see if we need the failback,

fallback

> just implement the .bdrv_co_create_opts in the drivers that need it.
> 
> This way we don't break various places that need to know if the underlying
> protocol/format really supports image creation, and this way we still
> allow some drivers to not support image creation.
> 
> Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007
> 
> Note that technically this driver reverts the image creation failback

fallback

> for the vxhs driver since I don't have a means to test it,
> and IMHO it is better to leave it not supported as it was prior to
> generic image creation patches.
> 
> Also drop iscsi_create_opts which was left accidently

accidentally

> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> +++ b/block/file-posix.c
> @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
>       .bdrv_reopen_prepare = raw_reopen_prepare,
>       .bdrv_reopen_commit  = raw_reopen_commit,
>       .bdrv_reopen_abort   = raw_reopen_abort,
> +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
> +    .create_opts         = &bdrv_create_opts_simple,

I'd drop the leading & for consistency with the rest of this struct 
initializer.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/2] block: pass BlockDriver reference to the .bdrv_co_create

[Maxim Levitsky]

On Thu, 2020-03-26 at 08:18 -0500, Eric Blake wrote:
> On 3/25/20 8:12 PM, Maxim Levitsky wrote:
> > This will allow to reuse a single generic .bdrv_co_create
> 
> "allow to ${verb}" is not idiomatic, better is "allow ${subject} to 
> ${verb}" or "allow ${verb}ing".  In this case, I'd go with:
> 
> This will allow the reuse of a single...
I agree! This commit will probably go as is but next time I'll keep it in mind!

Best regards,
	Maxim Levitsky

> 
> > implementation for several drivers.
> > No functional changes.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---

Re: [PATCH 1/2] block: pass BlockDriver reference to the .bdrv_co_create

[Eric Blake]

On 3/26/20 8:22 AM, Maxim Levitsky wrote:
> On Thu, 2020-03-26 at 08:18 -0500, Eric Blake wrote:
>> On 3/25/20 8:12 PM, Maxim Levitsky wrote:
>>> This will allow to reuse a single generic .bdrv_co_create
>>
>> "allow to ${verb}" is not idiomatic, better is "allow ${subject} to
>> ${verb}" or "allow ${verb}ing".  In this case, I'd go with:
>>
>> This will allow the reuse of a single...
> I agree! This commit will probably go as is but next time I'll keep it in mind!

Max hasn't sent the pull request yet; there's still time for him to 
amend his staging queue if he wants.  But yeah, it's not a huge deal if 
the patch goes in without spelling/grammar polish.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Kevin Wolf]

Am 26.03.2020 um 14:20 hat Eric Blake geschrieben:
> > +++ b/block/file-posix.c
> > @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
> >       .bdrv_reopen_prepare = raw_reopen_prepare,
> >       .bdrv_reopen_commit  = raw_reopen_commit,
> >       .bdrv_reopen_abort   = raw_reopen_abort,
> > +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
> > +    .create_opts         = &bdrv_create_opts_simple,
> 
> I'd drop the leading & for consistency with the rest of this struct
> initializer.

This one isn't a function pointer, so I think the & is necessary.

Kevin

Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Maxim Levitsky]

On Thu, 2020-03-26 at 08:20 -0500, Eric Blake wrote:
> On 3/25/20 8:12 PM, Maxim Levitsky wrote:
> > Instead of checking the .bdrv_co_create_opts to see if we need the failback,
> 
> fallback
100% true.
> 
> > just implement the .bdrv_co_create_opts in the drivers that need it.
> > 
> > This way we don't break various places that need to know if the underlying
> > protocol/format really supports image creation, and this way we still
> > allow some drivers to not support image creation.
> > 
> > Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007
> > 
> > Note that technically this driver reverts the image creation failback
> 
> fallback
> 
> > for the vxhs driver since I don't have a means to test it,
> > and IMHO it is better to leave it not supported as it was prior to
> > generic image creation patches.
> > 
> > Also drop iscsi_create_opts which was left accidently
> 
> accidentally
True. I did a spell check on the commit message, but I guess I updated it
afterward with this.

> 
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > +++ b/block/file-posix.c
> > @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
> >       .bdrv_reopen_prepare = raw_reopen_prepare,
> >       .bdrv_reopen_commit  = raw_reopen_commit,
> >       .bdrv_reopen_abort   = raw_reopen_abort,
> > +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
> > +    .create_opts         = &bdrv_create_opts_simple,
> 
> I'd drop the leading & for consistency with the rest of this struct 
> initializer.

Can I? This is struct reference and I think that only for function references,
the leading & is optional.


Best regards,
	Maxim Levitsky

Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Eric Blake]

On 3/26/20 8:28 AM, Kevin Wolf wrote:
> Am 26.03.2020 um 14:20 hat Eric Blake geschrieben:
>>> +++ b/block/file-posix.c
>>> @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
>>>        .bdrv_reopen_prepare = raw_reopen_prepare,
>>>        .bdrv_reopen_commit  = raw_reopen_commit,
>>>        .bdrv_reopen_abort   = raw_reopen_abort,
>>> +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
>>> +    .create_opts         = &bdrv_create_opts_simple,
>>
>> I'd drop the leading & for consistency with the rest of this struct
>> initializer.
> 
> This one isn't a function pointer, so I think the & is necessary.

Ah, right. Visual pattern-matching failed me, since I didn't read the 
actual types in the .h file.

Hmm - is it possible to write the patch in such a way that .create_opts 
can be left NULL when .bdrv_co_create_opts is bdrv_co_create_opts_simple?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/2] Fix the generic image creation code

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1263 bytes --]

On 26.03.20 13:23, Max Reitz wrote:
> On 26.03.20 02:12, Maxim Levitsky wrote:
>> The recent patches from Max Reitz allowed some block drivers to not
>> provide the .bdrv_co_create_opts and still allow qemu-img to
>> create/format images as long as the image is already existing
>> (that is the case with various block storage drivers like nbd/iscsi/nvme, etc)
>>
>> However it was found out that some places in the code depend on the
>> .bdrv_co_create_opts/.create_opts to be != NULL to decide if to allow
>> image creation.
>>
>> To avoid adding failback code to all these places, just make generic failback
>> code be used by the drivers that need it, so that for outside user, there
>> is no diffirence if failback was used or not.
>>
>> Best regards,
>> 	Maxim Levitsky
>>
>> Maxim Levitsky (2):
>>   block: pass BlockDriver reference to the .bdrv_co_create
>>   block: trickle down the fallback image creation function use to the
>>     block drivers
> 
> Thanks, fixed the function parameter alignment, moved the declarations
> from block.h into block_int.h, and applied the series to my block branch:

(And the spelling fixes suggested by Eric)

> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> Max
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1218 bytes --]

On 26.03.20 14:35, Eric Blake wrote:
> On 3/26/20 8:28 AM, Kevin Wolf wrote:
>> Am 26.03.2020 um 14:20 hat Eric Blake geschrieben:
>>>> +++ b/block/file-posix.c
>>>> @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
>>>>        .bdrv_reopen_prepare = raw_reopen_prepare,
>>>>        .bdrv_reopen_commit  = raw_reopen_commit,
>>>>        .bdrv_reopen_abort   = raw_reopen_abort,
>>>> +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
>>>> +    .create_opts         = &bdrv_create_opts_simple,
>>>
>>> I'd drop the leading & for consistency with the rest of this struct
>>> initializer.
>>
>> This one isn't a function pointer, so I think the & is necessary.
> 
> Ah, right. Visual pattern-matching failed me, since I didn't read the
> actual types in the .h file.
> 
> Hmm - is it possible to write the patch in such a way that .create_opts
> can be left NULL when .bdrv_co_create_opts is bdrv_co_create_opts_simple?

Setting .create_opts is actually the main point of this series, so we
don’t have to look for and fix all places that decide whether a block
driver is capable of image creation based on whether it’s set or not.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]