block: Split bdrv_open
PATCH 1/1 block: Split bdrv_open
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.
---
This patch applies on top of Christoph's RFC for the format/protocol split
block.c | 63 +++++++++++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 45 insertions(+), 18 deletions(-)
block.c +45 -18
@@ -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; }
Signature · 1 line
1.6.6.1
Quoted 10 lines · Kevin Wolf
bdrv_opencontains 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) andbdrv_do_open()for the code common for opening both images and files.Signed-off-by Kevin Wolf <[email protected]>---
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?
Am 13.04.2010 20:20, schrieb Christoph Hellwig:
Quoted 12 lines · Kevin Wolf
Quoted 10 lines
bdrv_opencontains 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) andbdrv_do_open()for the code common for opening both images and files.Signed-off-by Kevin Wolf <[email protected]>---
This patch applies on top of Christoph's RFC for the format/protocol split
Quoted 5 lines
I like this a lot. A few comments:
- why is
bdrv_do_openbelow 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.
Quoted 2 lines
- a "do" a function name is not very meaningfull - what about
bdrv_open_commoninstead?
Heh, did exactly this yesterday because I felt the same. It's just not pushed yet.
Quoted 1 line
- 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