Patchwork Replace calls of old bdrv_open

login
register
mail settings
Submitter Kevin Wolf
Date March 31, 2010, 12:46 p.m.
Message ID <1270039576-11968-1-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/49149/
State New
Headers show

Comments

Kevin Wolf - March 31, 2010, 12:46 p.m.
What is known today as bdrv_open2 becomes the new bdrv_open. All remaining
callers of the old function are converted to the new one. In some places they
even know the right format, so they should have used bdrv_open2 from the
beginning.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c       |   24 +++++++++---------------
 block.h       |    5 ++---
 block/qcow2.c |    4 ++--
 block/vmdk.c  |    2 +-
 block/vvfat.c |    5 ++++-
 hw/xen_disk.c |    2 +-
 monitor.c     |    2 +-
 qemu-img.c    |   16 ++++++++--------
 qemu-io.c     |    2 +-
 qemu-nbd.c    |    2 +-
 vl.c          |    2 +-
 11 files changed, 31 insertions(+), 35 deletions(-)
Juan Quintela - March 31, 2010, 1:07 p.m.
Kevin Wolf <kwolf@redhat.com> wrote:
> What is known today as bdrv_open2 becomes the new bdrv_open. All remaining
> callers of the old function are converted to the new one. In some places they
> even know the right format, so they should have used bdrv_open2 from the
> beginning.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I would preffer a:
bdrv_open() like yours with assert(drv != NULL).

bdrv_open_guess() for the current behaviour with NULL drv argument.

Why?  because I would like to have an easy way to grep for all places
that "guess" what driver should they use.  And as you found on the
patch, several places knew it and where using it out of laziness.

If I can have my pony, your patch is better than current behaviour :)

Later, Juan.
Juan Quintela - March 31, 2010, 1:08 p.m.
Kevin Wolf <kwolf@redhat.com> wrote:
> What is known today as bdrv_open2 becomes the new bdrv_open. All remaining
> callers of the old function are converted to the new one. In some places they
> even know the right format, so they should have used bdrv_open2 from the
> beginning.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I would preffer a:
bdrv_open() like yours with assert(drv != NULL).

bdrv_open_guess() for the current behaviour with NULL drv argument.
(name sucks but I can guet a better one at the momment).

Why?  because I would like to have an easy way to grep for all places
that "guess" what driver should they use.  And as you found on the
patch, several places knew it and where using it out of laziness.

If I can have my pony, your patch is better than current behaviour :)

Later, Juan.
Kevin Wolf - March 31, 2010, 1:12 p.m.
Am 31.03.2010 15:08, schrieb Juan Quintela:
> Kevin Wolf <kwolf@redhat.com> wrote:
>> What is known today as bdrv_open2 becomes the new bdrv_open. All remaining
>> callers of the old function are converted to the new one. In some places they
>> even know the right format, so they should have used bdrv_open2 from the
>> beginning.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I would preffer a:
> bdrv_open() like yours with assert(drv != NULL).
> 
> bdrv_open_guess() for the current behaviour with NULL drv argument.
> (name sucks but I can guet a better one at the momment).
> 
> Why?  because I would like to have an easy way to grep for all places
> that "guess" what driver should they use.  And as you found on the
> patch, several places knew it and where using it out of laziness.
> 
> If I can have my pony, your patch is better than current behaviour :)

Feel free to post a v2 that implements your version. :-)

Kevin
Christoph Hellwig - April 5, 2010, 8:55 a.m.
On Wed, Mar 31, 2010 at 02:46:16PM +0200, Kevin Wolf wrote:
> What is known today as bdrv_open2 becomes the new bdrv_open. All remaining
> callers of the old function are converted to the new one. In some places they
> even know the right format, so they should have used bdrv_open2 from the
> beginning.

Looks good.  I had a patch doing the same but wasn't sure if it's worth
bothering.
Christoph Hellwig - April 5, 2010, 8:58 a.m.
On Wed, Mar 31, 2010 at 03:12:56PM +0200, Kevin Wolf wrote:
> > If I can have my pony, your patch is better than current behaviour :)
> 
> Feel free to post a v2 that implements your version. :-)

No real need for that, it can also be done as a separate path on top
of this one.

Note that there's also another interesting aspect to this - currently
vl.c, monitor.c and the xen disk code uses bdrv_find_whitelisted_format,
but various other places that can be called directly from qemu and
not just qemu-img/qemu-io do the "full" guess.  Taking the whitelist
check into the guessing helper would sort this out.

Patch

diff --git a/block.c b/block.c
index 02f4258..31610af 100644
--- a/block.c
+++ b/block.c
@@ -338,7 +338,7 @@  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     int ret;
 
     bs = bdrv_new("");
-    ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, NULL);
+    ret = bdrv_open(bs, filename, flags | BDRV_O_FILE, NULL);
     if (ret < 0) {
         bdrv_delete(bs);
         return ret;
@@ -348,13 +348,8 @@  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     return 0;
 }
 
-int bdrv_open(BlockDriverState *bs, const char *filename, int flags)
-{
-    return bdrv_open2(bs, filename, flags, NULL);
-}
-
-int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
-               BlockDriver *drv)
+int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
+              BlockDriver *drv)
 {
     int ret, open_flags;
     char tmp_filename[PATH_MAX];
@@ -379,7 +374,7 @@  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 
         /* if there is a backing file, use it */
         bs1 = bdrv_new("");
-        ret = bdrv_open2(bs1, filename, 0, drv);
+        ret = bdrv_open(bs1, filename, 0, drv);
         if (ret < 0) {
             bdrv_delete(bs1);
             return ret;
@@ -491,9 +486,8 @@  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 
         /* backing files always opened read-only */
         open_flags &= ~BDRV_O_RDWR;
-        
-        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
-                         back_drv);
+
+        ret = bdrv_open(bs->backing_hd, backing_filename, open_flags, back_drv);
         if (ret < 0) {
             bdrv_close(bs);
             return ret;
@@ -605,12 +599,12 @@  int bdrv_commit(BlockDriverState *bs)
         bdrv_delete(bs->backing_hd);
         bs->backing_hd = NULL;
         bs_rw = bdrv_new("");
-        rw_ret = bdrv_open2(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
+        rw_ret = bdrv_open(bs_rw, filename, open_flags | BDRV_O_RDWR, NULL);
         if (rw_ret < 0) {
             bdrv_delete(bs_rw);
             /* try to re-open read-only */
             bs_ro = bdrv_new("");
-            ret = bdrv_open2(bs_ro, filename, open_flags & ~BDRV_O_RDWR, NULL);
+            ret = bdrv_open(bs_ro, filename, open_flags & ~BDRV_O_RDWR, NULL);
             if (ret < 0) {
                 bdrv_delete(bs_ro);
                 /* drive not functional anymore */
@@ -662,7 +656,7 @@  ro_cleanup:
         bdrv_delete(bs->backing_hd);
         bs->backing_hd = NULL;
         bs_ro = bdrv_new("");
-        ret = bdrv_open2(bs_ro, filename, open_flags & ~BDRV_O_RDWR, NULL);
+        ret = bdrv_open(bs_ro, filename, open_flags & ~BDRV_O_RDWR, NULL);
         if (ret < 0) {
             bdrv_delete(bs_ro);
             /* drive not functional anymore */
diff --git a/block.h b/block.h
index edf5704..8fca466 100644
--- a/block.h
+++ b/block.h
@@ -68,9 +68,8 @@  int bdrv_create2(BlockDriver *drv,
 BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
-int bdrv_open(BlockDriverState *bs, const char *filename, int flags);
-int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
-               BlockDriver *drv);
+int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
+              BlockDriver *drv);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_check(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/qcow2.c b/block/qcow2.c
index aca8844..4efe1ad 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -52,7 +52,7 @@  typedef struct {
 #define  QCOW_EXT_MAGIC_END 0
 #define  QCOW_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 
-
+static BlockDriver bdrv_qcow2;
 
 static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -1020,7 +1020,7 @@  exit:
     if (ret == 0 && prealloc) {
         BlockDriverState *bs;
         bs = bdrv_new("");
-        bdrv_open(bs, filename, BDRV_O_CACHE_WB | BDRV_O_RDWR);
+        bdrv_open(bs, filename, BDRV_O_CACHE_WB | BDRV_O_RDWR, &bdrv_qcow2);
         preallocate(bs);
         bdrv_close(bs);
     }
diff --git a/block/vmdk.c b/block/vmdk.c
index 007fca4..6fdea1d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -390,7 +390,7 @@  static int vmdk_parent_open(BlockDriverState *bs, const char * filename)
             return -1;
         }
         parent_open = 1;
-        if (bdrv_open(bs->backing_hd, parent_img_name, 0) < 0)
+        if (bdrv_open(bs->backing_hd, parent_img_name, 0, NULL) < 0)
             goto failure;
         parent_open = 0;
     }
diff --git a/block/vvfat.c b/block/vvfat.c
index 36f6ab4..0701df4 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2795,8 +2795,11 @@  static int enable_write_target(BDRVVVFATState *s)
     if (bdrv_create(bdrv_qcow, s->qcow_filename, options) < 0)
 	return -1;
     s->qcow = bdrv_new("");
-    if (s->qcow == NULL || bdrv_open(s->qcow, s->qcow_filename, BDRV_O_RDWR) < 0)
+    if (s->qcow == NULL ||
+        bdrv_open(s->qcow, s->qcow_filename, BDRV_O_RDWR, bdrv_qcow) < 0)
+    {
 	return -1;
+    }
 
 #ifndef _WIN32
     unlink(s->qcow_filename);
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index beadf90..95017a1 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -629,7 +629,7 @@  static int blk_init(struct XenDevice *xendev)
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
 	blkdev->bs = bdrv_new(blkdev->dev);
 	if (blkdev->bs) {
-	    if (bdrv_open2(blkdev->bs, blkdev->filename, qflags,
+	    if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
                            bdrv_find_whitelisted_format(blkdev->fileproto))
                 != 0) {
 		bdrv_delete(blkdev->bs);
diff --git a/monitor.c b/monitor.c
index 822dc27..a8ecea1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1096,7 +1096,7 @@  static int do_change_block(Monitor *mon, const char *device,
     if (eject_device(mon, bs, 0) < 0) {
         return -1;
     }
-    if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
+    if (bdrv_open(bs, filename, BDRV_O_RDWR, drv) < 0) {
         return -1;
     }
     return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
diff --git a/qemu-img.c b/qemu-img.c
index 66c8353..8c4f554 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -210,7 +210,7 @@  static BlockDriverState *bdrv_new_open(const char *filename,
     if (!readonly) {
         flags |= BDRV_O_RDWR;
     }
-    if (bdrv_open2(bs, filename, flags, drv) < 0) {
+    if (bdrv_open(bs, filename, flags, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     if (bdrv_is_encrypted(bs)) {
@@ -415,7 +415,7 @@  static int img_check(int argc, char **argv)
     } else {
         drv = NULL;
     }
-    if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) {
+    if (bdrv_open(bs, filename, BRDV_O_FLAGS, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     ret = bdrv_check(bs);
@@ -474,7 +474,7 @@  static int img_commit(int argc, char **argv)
     } else {
         drv = NULL;
     }
-    if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv) < 0) {
+    if (bdrv_open(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     ret = bdrv_commit(bs);
@@ -927,7 +927,7 @@  static int img_info(int argc, char **argv)
     } else {
         drv = NULL;
     }
-    if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_NO_BACKING, drv) < 0) {
+    if (bdrv_open(bs, filename, BRDV_O_FLAGS | BDRV_O_NO_BACKING, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     bdrv_get_format(bs, fmt_name, sizeof(fmt_name));
@@ -1033,7 +1033,7 @@  static int img_snapshot(int argc, char **argv)
     if (!bs)
         error("Not enough memory");
 
-    if (bdrv_open2(bs, filename, bdrv_oflags, NULL) < 0) {
+    if (bdrv_open(bs, filename, bdrv_oflags, NULL) < 0) {
         error("Could not open '%s'", filename);
     }
 
@@ -1138,7 +1138,7 @@  static int img_rebase(int argc, char **argv)
     }
 
     flags = BRDV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
-    if (bdrv_open2(bs, filename, flags, drv) < 0) {
+    if (bdrv_open(bs, filename, flags, drv) < 0) {
         error("Could not open '%s'", filename);
     }
 
@@ -1170,7 +1170,7 @@  static int img_rebase(int argc, char **argv)
 
         bs_old_backing = bdrv_new("old_backing");
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        if (bdrv_open2(bs_old_backing, backing_name, BRDV_O_FLAGS,
+        if (bdrv_open(bs_old_backing, backing_name, BRDV_O_FLAGS,
             old_backing_drv))
         {
             error("Could not open old backing file '%s'", backing_name);
@@ -1178,7 +1178,7 @@  static int img_rebase(int argc, char **argv)
         }
 
         bs_new_backing = bdrv_new("new_backing");
-        if (bdrv_open2(bs_new_backing, out_baseimg, BRDV_O_FLAGS | BDRV_O_RDWR,
+        if (bdrv_open(bs_new_backing, out_baseimg, BRDV_O_FLAGS | BDRV_O_RDWR,
             new_backing_drv))
         {
             error("Could not open new backing file '%s'", out_baseimg);
diff --git a/qemu-io.c b/qemu-io.c
index 6b83714..ffb5817 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1284,7 +1284,7 @@  static int openfile(char *name, int flags, int growable)
 		flags |= BDRV_O_FILE;
 	}
 
-	if (bdrv_open(bs, name, flags) < 0) {
+	if (bdrv_open(bs, name, flags, NULL) < 0) {
 		fprintf(stderr, "%s: can't open device %s\n", progname, name);
 		bs = NULL;
 		return 1;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6d854d3..25aa913 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -333,7 +333,7 @@  int main(int argc, char **argv)
     if (bs == NULL)
         return 1;
 
-    if (bdrv_open(bs, argv[optind], flags) < 0)
+    if (bdrv_open(bs, argv[optind], flags, NULL) < 0)
         return 1;
 
     fd_size = bs->total_sectors * 512;
diff --git a/vl.c b/vl.c
index 6768cf1..a66a5eb 100644
--- a/vl.c
+++ b/vl.c
@@ -1200,7 +1200,7 @@  DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     }
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
-    if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
+    if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
         fprintf(stderr, "qemu: could not open disk image %s: %s\n",
                         file, strerror(errno));
         return NULL;