diff mbox series

[2/3] block: do not check bdrv_file_open

Message ID 20221212131700.250209-3-pbonzini@redhat.com
State New
Headers show
Series block: remove separate bdrv_file_open callback | expand

Commit Message

Paolo Bonzini Dec. 12, 2022, 1:16 p.m. UTC 
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(-)

Comments

Kevin Wolf Jan. 19, 2023, 1:17 p.m. UTC | #1 
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
Paolo Bonzini April 21, 2023, 9:35 a.m. UTC | #2 
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
diff mbox series

Patch

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