Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static

Fri, 31 Jan 2014 12:22:38 -0800 

On 29.01.2014 14:26, Kevin Wolf wrote:

Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:

Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
therefore bdrv_open() the only way to call it.

Consequently, all existing calls to bdrv_file_open() have to be adjusted
to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.

Signed-off-by: Max Reitz <mre...@redhat.com>
---
  block.c               | 17 ++++++++++++-----
  block/cow.c           |  6 +++---
  block/qcow.c          |  6 +++---
  block/qcow2.c         |  5 +++--
  block/qed.c           |  5 +++--
  block/sheepdog.c      |  8 +++++---
  block/vhdx.c          |  5 +++--
  block/vmdk.c          | 11 +++++++----
  include/block/block.h |  5 ++---
  qemu-io.c             |  4 +++-
  10 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index f9923bb..0fb7892 100644
--- a/block.c
+++ b/block.c
@@ -947,9 +947,9 @@ free_and_fail:
   * after the call (even on failure), so if the caller intends to reuse the
   * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
   */
-int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                   const char *reference, QDict *options, int flags,
-                   Error **errp)
+static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+                          const char *reference, QDict *options, int flags,
+                          Error **errp)
  {
      BlockDriverState *bs = NULL;
      BlockDriver *drv;
@@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char 
*filename,
          *pbs = NULL;
          ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, 
errp);
      } else {
-        ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
-                             errp);
+        *pbs = NULL;
+        ret = bdrv_open(pbs, filename, reference, image_options,
+                        flags | BDRV_O_PROTOCOL, NULL, errp);
      }
done: 
@@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
      const char *drvname;
      Error *local_err = NULL;
+ if (flags & BDRV_O_PROTOCOL) { 
+        assert(!drv);
+        return bdrv_file_open(pbs, filename, reference, options,
+                              flags & ~BDRV_O_PROTOCOL, errp);
+    }
+
      /* NULL means an empty set of options */
      if (options == NULL) {
          options = qdict_new();
diff --git a/block/cow.c b/block/cow.c
index 7fc0b12..f0748a2 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -332,7 +332,7 @@ static int cow_create(const char *filename, 
QEMUOptionParameter *options,
      const char *image_filename = NULL;
      Error *local_err = NULL;
      int ret;
-    BlockDriverState *cow_bs;
+    BlockDriverState *cow_bs = NULL;
/* Read out options */ 
      while (options && options->name) {
@@ -351,8 +351,8 @@ static int cow_create(const char *filename, 
QEMUOptionParameter *options,
          return ret;
      }
- ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, 
-                         &local_err);
+    ret = bdrv_open(&cow_bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
      if (ret < 0) {
          qerror_report_err(local_err);
          error_free(local_err);
diff --git a/block/qcow.c b/block/qcow.c
index 948b0c5..4881d84 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -670,7 +670,7 @@ static int qcow_create(const char *filename, 
QEMUOptionParameter *options,
      int flags = 0;
      Error *local_err = NULL;
      int ret;
-    BlockDriverState *qcow_bs;
+    BlockDriverState *qcow_bs = NULL;
/* Read out options */ 
      while (options && options->name) {
@@ -691,8 +691,8 @@ static int qcow_create(const char *filename, 
QEMUOptionParameter *options,
          return ret;
      }
- ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR, 
-                         &local_err);
+    ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
      if (ret < 0) {
          qerror_report_err(local_err);
          error_free(local_err);
diff --git a/block/qcow2.c b/block/qcow2.c
index 7202a26..40b957b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1479,7 +1479,7 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
       * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
       * size for any qcow2 image.
       */
-    BlockDriverState* bs;
+    BlockDriverState *bs = NULL;
      QCowHeader *header;
      uint8_t* refcount_table;
      Error *local_err = NULL;
@@ -1491,7 +1491,8 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
          return ret;
      }
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); 
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
      if (ret < 0) {
          error_propagate(errp, local_err);
          return ret;
diff --git a/block/qed.c b/block/qed.c
index 694e6e2..dee2e61 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32_t 
cluster_size,
          return ret;
      }
- ret = bdrv_file_open(&bs, filename, NULL, NULL, 
-                         BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL,
+                    &local_err);
      if (ret < 0) {
          qerror_report_err(local_err);
          error_free(local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 672b9c9..6f2ec57 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename)
      Error *local_err = NULL;
      int ret;
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); 
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
      if (ret < 0) {
          qerror_report_err(local_err);
          error_free(local_err);
@@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, 
QEMUOptionParameter *options,
      }
if (backing_file) { 
-        BlockDriverState *bs;
+        BlockDriverState *bs = NULL;
          BDRVSheepdogState *base;
          BlockDriver *drv;
@@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, 
              goto out;
          }
- ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err); 
+        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
+                        &local_err);
          if (ret < 0) {
              qerror_report_err(local_err);
              error_free(local_err);
diff --git a/block/vhdx.c b/block/vhdx.c
index 9ee0a61..13513b4 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, 
QEMUOptionParameter *options,
gunichar2 *creator = NULL; 
      glong creator_items;
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
      const char *type = NULL;
      VHDXImageType image_type;
      Error *local_err = NULL;
@@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, 
QEMUOptionParameter *options,
          goto exit;
      }
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); 
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
      if (ret < 0) {
          error_propagate(errp, local_err);
          goto exit;
diff --git a/block/vmdk.c b/block/vmdk.c
index 37b2bc8..d2a69d5 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, 
BlockDriverState *bs,
path_combine(extent_path, sizeof(extent_path), 
                  desc_file_path, fname);
-        ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
-                             bs->open_flags, errp);
+        extent_file = NULL;
+        ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
+                        bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
          if (ret) {
              return ret;
          }
@@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filename, 
int64_t filesize,
          goto exit;
      }
- ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err); 
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
      if (ret < 0) {
          error_propagate(errp, local_err);
          goto exit;
@@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, 
QEMUOptionParameter *options,
              goto exit;
          }
      }
-    ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, 
&local_err);
+    ret = bdrv_open(&new_bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
      if (ret < 0) {
          error_setg_errno(errp, -ret, "Could not write description");
          goto exit;
diff --git a/include/block/block.h b/include/block/block.h
index a421041..396f9ed 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -102,6 +102,8 @@ typedef enum {
  #define BDRV_O_CHECK       0x1000  /* open solely for consistency check */
  #define BDRV_O_ALLOW_RDWR  0x2000  /* allow reopen to change from r/o to r/w 
*/
  #define BDRV_O_UNMAP       0x4000  /* execute guest UNMAP/TRIM operations */
+#define BDRV_O_PROTOCOL    0x8000  /* open the file using a protocol instead of
+                                      a block driver */

Protocol drivers are a subset of block drivers, so this description
doesn't make sense.
Hm, technically they probably are but it always seemed to me that bdrv_open() would never directly use a protocol, instead using raw as the format if no format was found.
More importantly, it is actually possible to use a non-protocol block driver with BDRV_O_PROTOCOL; it just needs to be explicitly specified. 


I guess we need to list the differences between bdrv_open() and
bdrv_file_open() in order to define what this flag really changes. I
think this includes:

- Disables format probing
- BDRV_O_SNAPSHOT is ignored
- No backing files are opened
- Probably a few more
So, to me, the main difference is that bdrv_open() always uses some non-protocol block driver, whereas bdrv_file_open() only probes for protocol block drivers (if a non-protocol driver should be used, it has to be explicitly specified).
The current comment for bdrv_file_open() doesn't help much, either: “Opens a file using a protocol”. Therefore, the easiest way would just be to state “behaves like the old bdrv_file_open()”, but that would not be very helpful. Perhaps we could formulate it like “Opens a single file (no backing chain, etc.) using only a protocol driver deduced from the filename, if not explicitly specified otherwise.” 

Max

Reply via email to