[PATCH 0/3] block: remove separate bdrv_file_open callback

[PATCH 0/3] block: remove separate bdrv_file_open callback

[Paolo Bonzini]

The presence of the bdrv_file_open callback is used in some parts of the
code to distinguish protocol and format drivers.  Use the existing
.protocol_name field instead, and unify .bdrv_open with .bdrv_file_open.

Paolo

Paolo Bonzini (3):
  block: apply assertion more widely
  block: do not check bdrv_file_open
  block: remove separate bdrv_file_open callback

 block.c                          | 17 +++++++----------
 block/blkdebug.c                 |  2 +-
 block/blkio.c                    |  2 +-
 block/blkverify.c                |  2 +-
 block/curl.c                     |  8 ++++----
 block/file-posix.c               |  8 ++++----
 block/file-win32.c               |  4 ++--
 block/gluster.c                  |  8 ++++----
 block/iscsi.c                    |  4 ++--
 block/nbd.c                      |  6 +++---
 block/nfs.c                      |  2 +-
 block/null.c                     |  4 ++--
 block/nvme.c                     |  2 +-
 block/rbd.c                      |  3 ++-
 block/ssh.c                      |  2 +-
 block/vvfat.c                    |  2 +-
 include/block/block_int-common.h |  3 ---
 17 files changed, 37 insertions(+), 42 deletions(-)

-- 
2.38.1

[PATCH 1/3] block: apply assertion more widely

[Paolo Bonzini]

.bdrv_needs_filename is only set for drivers that also set bdrv_file_open,
i.e. protocol drivers.

So we can make the assertion always, it will always pass for those drivers
that use bdrv_open.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 676bbe0529b0..0a625a489a6e 100644
--- a/block.c
+++ b/block.c
@@ -1617,8 +1617,8 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
+    assert(!drv->bdrv_needs_filename || bs->filename[0]);
     if (drv->bdrv_file_open) {
-        assert(!drv->bdrv_needs_filename || bs->filename[0]);
         ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
     } else if (drv->bdrv_open) {
         ret = drv->bdrv_open(bs, options, open_flags, &local_err);
-- 
2.38.1

[PATCH 2/3] block: do not check bdrv_file_open

[Paolo Bonzini]

The set of BlockDrivers that have .bdrv_file_open coincides with those
that have .protocol_name and guess what---checking drv->bdrv_file_open
is done to see if the driver is a protocol.  So check drv->protocol_name
instead.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 0a625a489a6e..7a66cc2ea23a 100644
--- a/block.c
+++ b/block.c
@@ -911,7 +911,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
     int i;
 
     GLOBAL_STATE_CODE();
-    /* TODO Drivers without bdrv_file_open must be specified explicitly */
 
     /*
      * XXX(hch): we really should not let host device detection
@@ -1618,7 +1617,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
     bs->opaque = g_malloc0(drv->instance_size);
 
     assert(!drv->bdrv_needs_filename || bs->filename[0]);
-    if (drv->bdrv_file_open) {
+    if (drv->bdrv_open) {
         ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
     } else if (drv->bdrv_open) {
         ret = drv->bdrv_open(bs, options, open_flags, &local_err);
@@ -1930,7 +1929,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     open_flags = bdrv_open_flags(bs, bs->open_flags);
     node_name = qemu_opt_get(opts, "node-name");
 
-    assert(!drv->bdrv_file_open || file == NULL);
+    assert(!drv->protocol_name || file == NULL);
     ret = bdrv_open_driver(bs, drv, node_name, options, open_flags, errp);
     if (ret < 0) {
         goto fail_opts;
@@ -2030,7 +2029,7 @@ static int bdrv_fill_options(QDict **options, const char *filename,
         }
         /* If the user has explicitly specified the driver, this choice should
          * override the BDRV_O_PROTOCOL flag */
-        protocol = drv->bdrv_file_open;
+        protocol = drv->protocol_name;
     }
 
     if (protocol) {
@@ -3932,7 +3931,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     }
 
     /* BDRV_O_PROTOCOL must be set iff a protocol BDS is about to be created */
-    assert(!!(flags & BDRV_O_PROTOCOL) == !!drv->bdrv_file_open);
+    assert(!!(flags & BDRV_O_PROTOCOL) == !!drv->protocol_name);
     /* file must be NULL if a protocol BDS is about to be created
      * (the inverse results in an error message from bdrv_open_common()) */
     assert(!(flags & BDRV_O_PROTOCOL) || !file);
@@ -5671,7 +5670,7 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
         return drv->bdrv_get_allocated_file_size(bs);
     }
 
-    if (drv->bdrv_file_open) {
+    if (drv->protocol_name) {
         /*
          * Protocol drivers default to -ENOTSUP (most of their data is
          * not stored in any of their children (if they even have any),
@@ -7772,7 +7771,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
          *   Both of these conditions are represented by generate_json_filename.
          */
         if (primary_child_bs->exact_filename[0] &&
-            primary_child_bs->drv->bdrv_file_open &&
+            primary_child_bs->drv->protocol_name &&
             !drv->is_filter && !generate_json_filename)
         {
             strcpy(bs->exact_filename, primary_child_bs->exact_filename);
-- 
2.38.1

Re: [PATCH 2/3] block: do not check bdrv_file_open

[Kevin Wolf]

Am 12.12.2022 um 14:16 hat Paolo Bonzini geschrieben:
> The set of BlockDrivers that have .bdrv_file_open coincides with those
> that have .protocol_name and guess what---checking drv->bdrv_file_open
> is done to see if the driver is a protocol.  So check drv->protocol_name
> instead.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0a625a489a6e..7a66cc2ea23a 100644
> --- a/block.c
> +++ b/block.c
> @@ -911,7 +911,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>      int i;
>  
>      GLOBAL_STATE_CODE();
> -    /* TODO Drivers without bdrv_file_open must be specified explicitly */
>  
>      /*
>       * XXX(hch): we really should not let host device detection
> @@ -1618,7 +1617,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>      bs->opaque = g_malloc0(drv->instance_size);
>  
>      assert(!drv->bdrv_needs_filename || bs->filename[0]);
> -    if (drv->bdrv_file_open) {
> +    if (drv->bdrv_open) {
>          ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
>      } else if (drv->bdrv_open) {
>          ret = drv->bdrv_open(bs, options, open_flags, &local_err);

I suppose you mean drv->protocol_name for the first if condition?

The bug will disappear again after patch 3, but this intermediate state
is very broken.

Kevin

Re: [PATCH 2/3] block: do not check bdrv_file_open

[Paolo Bonzini]

On Thu, Jan 19, 2023 at 2:17 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >      assert(!drv->bdrv_needs_filename || bs->filename[0]);
> > -    if (drv->bdrv_file_open) {
> > +    if (drv->bdrv_open) {
> >          ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
> >      } else if (drv->bdrv_open) {
> >          ret = drv->bdrv_open(bs, options, open_flags, &local_err);
>
> I suppose you mean drv->protocol_name for the first if condition?
>
> The bug will disappear again after patch 3, but this intermediate state
> is very broken.

Yep, I split the patch wrong. Will resend after you merge block-next.

Paolo

[PATCH 3/3] block: remove separate bdrv_file_open callback

[Paolo Bonzini]

bdrv_file_open and bdrv_open are completely equivalent, they are
never checked except to see which one to invoke.  So merge them
into a single callback.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c                          | 2 --
 block/blkdebug.c                 | 2 +-
 block/blkio.c                    | 2 +-
 block/blkverify.c                | 2 +-
 block/curl.c                     | 8 ++++----
 block/file-posix.c               | 8 ++++----
 block/file-win32.c               | 4 ++--
 block/gluster.c                  | 8 ++++----
 block/iscsi.c                    | 4 ++--
 block/nbd.c                      | 6 +++---
 block/nfs.c                      | 2 +-
 block/null.c                     | 4 ++--
 block/nvme.c                     | 2 +-
 block/rbd.c                      | 3 ++-
 block/ssh.c                      | 2 +-
 block/vvfat.c                    | 2 +-
 include/block/block_int-common.h | 3 ---
 17 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/block.c b/block.c
index 7a66cc2ea23a..48efbf94106e 100644
--- a/block.c
+++ b/block.c
@@ -1618,8 +1618,6 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
 
     assert(!drv->bdrv_needs_filename || bs->filename[0]);
     if (drv->bdrv_open) {
-        ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
-    } else if (drv->bdrv_open) {
         ret = drv->bdrv_open(bs, options, open_flags, &local_err);
     } else {
         ret = 0;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 4265ca125e25..a4445a352876 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -1071,7 +1071,7 @@ static BlockDriver bdrv_blkdebug = {
     .is_filter              = true,
 
     .bdrv_parse_filename    = blkdebug_parse_filename,
-    .bdrv_file_open         = blkdebug_open,
+    .bdrv_open              = blkdebug_open,
     .bdrv_close             = blkdebug_close,
     .bdrv_reopen_prepare    = blkdebug_reopen_prepare,
     .bdrv_child_perm        = blkdebug_child_perm,
diff --git a/block/blkio.c b/block/blkio.c
index 5eae3adfaf66..45ae5780152c 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -994,7 +994,7 @@ static void blkio_refresh_limits(BlockDriverState *bs, Error **errp)
         .format_name             = name, \
         .protocol_name           = name, \
         .instance_size           = sizeof(BDRVBlkioState), \
-        .bdrv_file_open          = blkio_file_open, \
+        .bdrv_open               = blkio_open, \
         .bdrv_close              = blkio_close, \
         .bdrv_getlength          = blkio_getlength, \
         .bdrv_co_truncate        = blkio_truncate, \
diff --git a/block/blkverify.c b/block/blkverify.c
index c60a2dc624dc..e5b5d5c4a426 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -310,7 +310,7 @@ static BlockDriver bdrv_blkverify = {
     .instance_size                    = sizeof(BDRVBlkverifyState),
 
     .bdrv_parse_filename              = blkverify_parse_filename,
-    .bdrv_file_open                   = blkverify_open,
+    .bdrv_open                        = blkverify_open,
     .bdrv_close                       = blkverify_close,
     .bdrv_child_perm                  = bdrv_default_perms,
     .bdrv_getlength                   = blkverify_getlength,
diff --git a/block/curl.c b/block/curl.c
index cba4c4cac768..398b28163e00 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -999,7 +999,7 @@ static BlockDriver bdrv_http = {
 
     .instance_size              = sizeof(BDRVCURLState),
     .bdrv_parse_filename        = curl_parse_filename,
-    .bdrv_file_open             = curl_open,
+    .bdrv_open                  = curl_open,
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
@@ -1018,7 +1018,7 @@ static BlockDriver bdrv_https = {
 
     .instance_size              = sizeof(BDRVCURLState),
     .bdrv_parse_filename        = curl_parse_filename,
-    .bdrv_file_open             = curl_open,
+    .bdrv_open                  = curl_open,
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
@@ -1037,7 +1037,7 @@ static BlockDriver bdrv_ftp = {
 
     .instance_size              = sizeof(BDRVCURLState),
     .bdrv_parse_filename        = curl_parse_filename,
-    .bdrv_file_open             = curl_open,
+    .bdrv_open                  = curl_open,
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
@@ -1056,7 +1056,7 @@ static BlockDriver bdrv_ftps = {
 
     .instance_size              = sizeof(BDRVCURLState),
     .bdrv_parse_filename        = curl_parse_filename,
-    .bdrv_file_open             = curl_open,
+    .bdrv_open                  = curl_open,
     .bdrv_close                 = curl_close,
     .bdrv_getlength             = curl_getlength,
 
diff --git a/block/file-posix.c b/block/file-posix.c
index b9647c5ffc26..91c4a2d74687 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3301,7 +3301,7 @@ BlockDriver bdrv_file = {
     .bdrv_needs_filename = true,
     .bdrv_probe = NULL, /* no probe for protocols */
     .bdrv_parse_filename = raw_parse_filename,
-    .bdrv_file_open = raw_open,
+    .bdrv_open      = raw_open,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit = raw_reopen_commit,
     .bdrv_reopen_abort = raw_reopen_abort,
@@ -3675,7 +3675,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_needs_filename = true,
     .bdrv_probe_device  = hdev_probe_device,
     .bdrv_parse_filename = hdev_parse_filename,
-    .bdrv_file_open     = hdev_open,
+    .bdrv_open          = hdev_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
@@ -3803,7 +3803,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_needs_filename = true,
     .bdrv_probe_device	= cdrom_probe_device,
     .bdrv_parse_filename = cdrom_parse_filename,
-    .bdrv_file_open     = cdrom_open,
+    .bdrv_open          = cdrom_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
@@ -3934,7 +3934,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_needs_filename = true,
     .bdrv_probe_device	= cdrom_probe_device,
     .bdrv_parse_filename = cdrom_parse_filename,
-    .bdrv_file_open     = cdrom_open,
+    .bdrv_open          = cdrom_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
diff --git a/block/file-win32.c b/block/file-win32.c
index ec9d64d0e4e2..4207e521f22a 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -748,7 +748,7 @@ BlockDriver bdrv_file = {
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_needs_filename = true,
     .bdrv_parse_filename = raw_parse_filename,
-    .bdrv_file_open     = raw_open,
+    .bdrv_open          = raw_open,
     .bdrv_refresh_limits = raw_probe_alignment,
     .bdrv_close         = raw_close,
     .bdrv_co_create_opts = raw_co_create_opts,
@@ -921,7 +921,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_needs_filename = true,
     .bdrv_parse_filename = hdev_parse_filename,
     .bdrv_probe_device	= hdev_probe_device,
-    .bdrv_file_open	= hdev_open,
+    .bdrv_open     	= hdev_open,
     .bdrv_close		= raw_close,
     .bdrv_refresh_limits = hdev_refresh_limits,
 
diff --git a/block/gluster.c b/block/gluster.c
index 7c90f7ba4b80..0ab196b35229 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1554,7 +1554,7 @@ static BlockDriver bdrv_gluster = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_open                    = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
     .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
@@ -1583,7 +1583,7 @@ static BlockDriver bdrv_gluster_tcp = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster+tcp",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_open                    = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
     .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
@@ -1612,7 +1612,7 @@ static BlockDriver bdrv_gluster_unix = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster+unix",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_open                    = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
     .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
@@ -1647,7 +1647,7 @@ static BlockDriver bdrv_gluster_rdma = {
     .format_name                  = "gluster",
     .protocol_name                = "gluster+rdma",
     .instance_size                = sizeof(BDRVGlusterState),
-    .bdrv_file_open               = qemu_gluster_open,
+    .bdrv_open                    = qemu_gluster_open,
     .bdrv_reopen_prepare          = qemu_gluster_reopen_prepare,
     .bdrv_reopen_commit           = qemu_gluster_reopen_commit,
     .bdrv_reopen_abort            = qemu_gluster_reopen_abort,
diff --git a/block/iscsi.c b/block/iscsi.c
index a316d46d9625..dc6cbe0f74a5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2425,7 +2425,7 @@ static BlockDriver bdrv_iscsi = {
 
     .instance_size          = sizeof(IscsiLun),
     .bdrv_parse_filename    = iscsi_parse_filename,
-    .bdrv_file_open         = iscsi_open,
+    .bdrv_open              = iscsi_open,
     .bdrv_close             = iscsi_close,
     .bdrv_co_create_opts    = bdrv_co_create_opts_simple,
     .create_opts            = &bdrv_create_opts_simple,
@@ -2464,7 +2464,7 @@ static BlockDriver bdrv_iser = {
 
     .instance_size          = sizeof(IscsiLun),
     .bdrv_parse_filename    = iscsi_parse_filename,
-    .bdrv_file_open         = iscsi_open,
+    .bdrv_open              = iscsi_open,
     .bdrv_close             = iscsi_close,
     .bdrv_co_create_opts    = bdrv_co_create_opts_simple,
     .create_opts            = &bdrv_create_opts_simple,
diff --git a/block/nbd.c b/block/nbd.c
index 7d485c86d285..a332372a5380 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -2114,7 +2114,7 @@ static BlockDriver bdrv_nbd = {
     .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_open                  = nbd_open,
     .bdrv_reopen_prepare        = nbd_client_reopen_prepare,
     .bdrv_co_preadv             = nbd_client_co_preadv,
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
@@ -2142,7 +2142,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .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_open                  = nbd_open,
     .bdrv_reopen_prepare        = nbd_client_reopen_prepare,
     .bdrv_co_preadv             = nbd_client_co_preadv,
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
@@ -2170,7 +2170,7 @@ static BlockDriver bdrv_nbd_unix = {
     .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_open                  = nbd_open,
     .bdrv_reopen_prepare        = nbd_client_reopen_prepare,
     .bdrv_co_preadv             = nbd_client_co_preadv,
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
diff --git a/block/nfs.c b/block/nfs.c
index ece22353ac53..126e3898f966 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -888,7 +888,7 @@ static BlockDriver bdrv_nfs = {
 #endif
     .bdrv_co_truncate               = nfs_file_co_truncate,
 
-    .bdrv_file_open                 = nfs_file_open,
+    .bdrv_open                      = nfs_file_open,
     .bdrv_close                     = nfs_file_close,
     .bdrv_co_create                 = nfs_file_co_create,
     .bdrv_co_create_opts            = nfs_file_co_create_opts,
diff --git a/block/null.c b/block/null.c
index 75f7d0db40c4..f3a427b6c69a 100644
--- a/block/null.c
+++ b/block/null.c
@@ -281,7 +281,7 @@ static BlockDriver bdrv_null_co = {
     .protocol_name          = "null-co",
     .instance_size          = sizeof(BDRVNullState),
 
-    .bdrv_file_open         = null_file_open,
+    .bdrv_open              = null_file_open,
     .bdrv_parse_filename    = null_co_parse_filename,
     .bdrv_getlength         = null_getlength,
     .bdrv_get_allocated_file_size = null_allocated_file_size,
@@ -302,7 +302,7 @@ static BlockDriver bdrv_null_aio = {
     .protocol_name          = "null-aio",
     .instance_size          = sizeof(BDRVNullState),
 
-    .bdrv_file_open         = null_file_open,
+    .bdrv_open              = null_file_open,
     .bdrv_parse_filename    = null_aio_parse_filename,
     .bdrv_getlength         = null_getlength,
     .bdrv_get_allocated_file_size = null_allocated_file_size,
diff --git a/block/nvme.c b/block/nvme.c
index 656624c585a2..5083b36b89bc 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1640,7 +1640,7 @@ static BlockDriver bdrv_nvme = {
     .create_opts              = &bdrv_create_opts_simple,
 
     .bdrv_parse_filename      = nvme_parse_filename,
-    .bdrv_file_open           = nvme_file_open,
+    .bdrv_open                = nvme_file_open,
     .bdrv_close               = nvme_close,
     .bdrv_getlength           = nvme_getlength,
     .bdrv_probe_blocksizes    = nvme_probe_blocksizes,
diff --git a/block/rbd.c b/block/rbd.c
index f826410f40f9..4c759dd51167 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1648,8 +1648,9 @@ static const char *const qemu_rbd_strong_runtime_opts[] = {
 static BlockDriver bdrv_rbd = {
     .format_name            = "rbd",
     .instance_size          = sizeof(BDRVRBDState),
+
     .bdrv_parse_filename    = qemu_rbd_parse_filename,
-    .bdrv_file_open         = qemu_rbd_open,
+    .bdrv_open              = qemu_rbd_open,
     .bdrv_close             = qemu_rbd_close,
     .bdrv_reopen_prepare    = qemu_rbd_reopen_prepare,
     .bdrv_co_create         = qemu_rbd_co_create,
diff --git a/block/ssh.c b/block/ssh.c
index 04726d4ecb58..07a99d6b8ed6 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -1356,7 +1356,7 @@ static BlockDriver bdrv_ssh = {
     .protocol_name                = "ssh",
     .instance_size                = sizeof(BDRVSSHState),
     .bdrv_parse_filename          = ssh_parse_filename,
-    .bdrv_file_open               = ssh_file_open,
+    .bdrv_open                    = ssh_file_open,
     .bdrv_co_create               = ssh_co_create,
     .bdrv_co_create_opts          = ssh_co_create_opts,
     .bdrv_close                   = ssh_close,
diff --git a/block/vvfat.c b/block/vvfat.c
index 723c91216e0c..88943a99db9d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3257,7 +3257,7 @@ static BlockDriver bdrv_vvfat = {
     .instance_size          = sizeof(BDRVVVFATState),
 
     .bdrv_parse_filename    = vvfat_parse_filename,
-    .bdrv_file_open         = vvfat_open,
+    .bdrv_open              = vvfat_open,
     .bdrv_refresh_limits    = vvfat_refresh_limits,
     .bdrv_close             = vvfat_close,
     .bdrv_child_perm        = vvfat_child_perm,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index a6bc6b7fe9bd..c670f3c8da1c 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -245,9 +245,6 @@ struct BlockDriver {
     int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp);
 
-    /* Protocol drivers should implement this instead of bdrv_open */
-    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
-                          Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
 
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
-- 
2.38.1

[PATCH 0/3] block: remove separate bdrv_file_open callback

[Paolo Bonzini]

The value of the bdrv_file_open is sometimes checked to distinguish
protocol and format drivers, but apart from that there is no difference
between bdrv_file_open and bdrv_open.

However, they can all be distinguished by the non-NULL .protocol_name
member.  Change the checks to use .protocol_name instead of .bdrv_file_open,
and unify the two callbacks.

Paolo

Paolo Bonzini (3):
  block: make assertion more generic
  block: do not check bdrv_file_open
  block: remove separate bdrv_file_open callback

 block.c                          | 17 +++++++----------
 block/blkdebug.c                 |  2 +-
 block/blkio.c                    |  2 +-
 block/blkverify.c                |  2 +-
 block/curl.c                     |  8 ++++----
 block/file-posix.c               |  8 ++++----
 block/file-win32.c               |  4 ++--
 block/gluster.c                  |  8 ++++----
 block/iscsi.c                    |  4 ++--
 block/nbd.c                      |  6 +++---
 block/nfs.c                      |  2 +-
 block/null.c                     |  4 ++--
 block/nvme.c                     |  2 +-
 block/rbd.c                      |  3 ++-
 block/ssh.c                      |  2 +-
 block/vvfat.c                    |  2 +-
 include/block/block_int-common.h |  3 ---
 17 files changed, 37 insertions(+), 42 deletions(-)

-- 
2.39.2

Re: [PATCH 0/3] block: remove separate bdrv_file_open callback

[Philippe Mathieu-Daudé]

On 9/3/23 09:50, Paolo Bonzini wrote:
> The value of the bdrv_file_open is sometimes checked to distinguish
> protocol and format drivers, but apart from that there is no difference
> between bdrv_file_open and bdrv_open.
> 
> However, they can all be distinguished by the non-NULL .protocol_name
> member.  Change the checks to use .protocol_name instead of .bdrv_file_open,
> and unify the two callbacks.

Nice cleanup. Series:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Paolo Bonzini (3):
>    block: make assertion more generic
>    block: do not check bdrv_file_open
>    block: remove separate bdrv_file_open callback

Re: [PATCH 0/3] block: remove separate bdrv_file_open callback

[Paolo Bonzini]

On Thu, Mar 9, 2023 at 9:50 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> The value of the bdrv_file_open is sometimes checked to distinguish
> protocol and format drivers, but apart from that there is no difference
> between bdrv_file_open and bdrv_open.
>
> However, they can all be distinguished by the non-NULL .protocol_name
> member.  Change the checks to use .protocol_name instead of .bdrv_file_open,
> and unify the two callbacks.

Ping.

Paolo