[Qemu-devel] [PATCH] block: Split bdrv_open

[Qemu-devel] [PATCH] block: Split bdrv_open

[Kevin Wolf]

bdrv_open contains quite some code that is only useful for opening images (as
opposed to opening files by a protocol), for example snapshots.

This patch splits the code so that we have bdrv_open_file() for files (uses
protocols), bdrv_open() for images (uses format drivers) and bdrv_do_open() for
the code common for opening both images and files.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
This patch applies on top of Christoph's RFC for the format/protocol split

 block.c |   63 +++++++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index 0c27f30..4bab221 100644
--- a/block.c
+++ b/block.c
@@ -42,6 +42,9 @@
 #include <windows.h>
 #endif
 
+static int bdrv_do_open(BlockDriverState *bs, const char *filename, int flags,
+    BlockDriver *drv);
+
 static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
@@ -335,6 +338,7 @@ static BlockDriver *find_image_format(const char *filename)
     return drv;
 }
 
+/* Opens a file using a protocol (file, host_device, nbd, ...) */
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
 {
     BlockDriverState *bs;
@@ -347,7 +351,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     }
 
     bs = bdrv_new("");
-    ret = bdrv_open(bs, filename, flags, drv);
+    ret = bdrv_do_open(bs, filename, flags, drv);
     if (ret < 0) {
         bdrv_delete(bs);
         return ret;
@@ -357,19 +361,11 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     return 0;
 }
 
+/* Opens a disk image (raw, qcow2, vmdk, ...) */
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv)
 {
-    int ret, open_flags;
-    char tmp_filename[PATH_MAX];
-    char backing_filename[PATH_MAX];
-
-    bs->is_temporary = 0;
-    bs->encrypted = 0;
-    bs->valid_key = 0;
-    bs->open_flags = flags;
-    /* buffer_alignment defaulted to 512, drivers can change this value */
-    bs->buffer_alignment = 512;
+    int ret;
 
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
@@ -377,6 +373,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         int is_protocol = 0;
         BlockDriver *bdrv_qcow2;
         QEMUOptionParameter *options;
+        char tmp_filename[PATH_MAX];
+        char backing_filename[PATH_MAX];
 
         /* if snapshot, we create a temporary backing file and open it
            instead of opening 'filename' directly */
@@ -424,8 +422,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         bs->is_temporary = 1;
     }
 
-    pstrcpy(bs->filename, sizeof(bs->filename), filename);
-
+    /* Find the right image format driver */
     if (!drv) {
         drv = find_image_format(filename);
     }
@@ -434,11 +431,44 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         ret = -ENOENT;
         goto unlink_and_fail;
     }
-    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
-        ret = -ENOTSUP;
+
+    /* Open the image */
+    ret = bdrv_do_open(bs, filename, flags, drv);
+    if (ret < 0) {
         goto unlink_and_fail;
     }
 
+    return 0;
+
+unlink_and_fail:
+    if (bs->is_temporary) {
+        unlink(filename);
+    }
+    return ret;
+}
+
+/* Common part for opening disk images and files */
+static int bdrv_do_open(BlockDriverState *bs, const char *filename, int flags,
+    BlockDriver *drv)
+{
+    int ret, open_flags;
+    char backing_filename[PATH_MAX];
+
+    assert(drv != NULL);
+
+    bs->is_temporary = 0;
+    bs->encrypted = 0;
+    bs->valid_key = 0;
+    bs->open_flags = flags;
+    /* buffer_alignment defaulted to 512, drivers can change this value */
+    bs->buffer_alignment = 512;
+
+    pstrcpy(bs->filename, sizeof(bs->filename), filename);
+
+    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
+        return -ENOTSUP;
+    }
+
     bs->drv = drv;
     bs->opaque = qemu_mallocz(drv->instance_size);
 
@@ -515,9 +545,6 @@ free_and_fail:
     qemu_free(bs->opaque);
     bs->opaque = NULL;
     bs->drv = NULL;
-unlink_and_fail:
-    if (bs->is_temporary)
-        unlink(filename);
     return ret;
 }
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] block: Split bdrv_open

[Christoph Hellwig]

On Mon, Apr 12, 2010 at 04:49:16PM +0200, Kevin Wolf wrote:
> bdrv_open contains quite some code that is only useful for opening images (as
> opposed to opening files by a protocol), for example snapshots.
> 
> This patch splits the code so that we have bdrv_open_file() for files (uses
> protocols), bdrv_open() for images (uses format drivers) and bdrv_do_open() for
> the code common for opening both images and files.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> This patch applies on top of Christoph's RFC for the format/protocol split

I like this a lot.  A few comments:

 - why is bdrv_do_open below it's users in the code?  I really hate
   forward declarations of functions and they can usually be easily
   avoided.
 - a "do" a function name is not very meaningfull - what about
   bdrv_open_common instead?
 - doesn't the backing device handling only apply to image formats, too?

[Qemu-devel] Re: [PATCH] block: Split bdrv_open

[Kevin Wolf]

Am 13.04.2010 20:20, schrieb Christoph Hellwig:
> On Mon, Apr 12, 2010 at 04:49:16PM +0200, Kevin Wolf wrote:
>> bdrv_open contains quite some code that is only useful for opening images (as
>> opposed to opening files by a protocol), for example snapshots.
>>
>> This patch splits the code so that we have bdrv_open_file() for files (uses
>> protocols), bdrv_open() for images (uses format drivers) and bdrv_do_open() for
>> the code common for opening both images and files.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> This patch applies on top of Christoph's RFC for the format/protocol split
> 
> I like this a lot.  A few comments:
> 
>  - why is bdrv_do_open below it's users in the code?  I really hate
>    forward declarations of functions and they can usually be easily
>    avoided.

Ok, I'll move it.

>  - a "do" a function name is not very meaningfull - what about
>    bdrv_open_common instead?

Heh, did exactly this yesterday because I felt the same. It's just not
pushed yet.

>  - doesn't the backing device handling only apply to image formats, too?

Hm, probably yes. At least currently no protocol uses it. We could add
an assert(bs->backing_hd == NULL) to bdrv_file_open to make clear that
backing files are not wanted for protocols.

Kevin