block: clean up bdrv_open2 structure a bit
PATCH 1/2 block: clean up bdrv_open2 structure a bit
Check the whitelist as early as possible instead of continuing the setup, and move all the error handling code to the end of the function.
block.c +20 -11
@@ -428,10 +428,16 @@ int bdrv_open2(BlockDriverState *bs, con drv =find_image_format (filename); } } +if (!drv) { ret = -ENOENT ; goto unlink_and_fail; } +if (use_bdrv_whitelist && !bdrv_is_whitelisted (drv)) { + ret = -ENOTSUP ; + goto unlink_and_fail; + } + bs->drv = drv; bs->opaque =qemu_mallocz (drv->instance_size);
@@ -452,23 +458,17 @@ int bdrv_open2(BlockDriverState *bs, con (flags & (BDRV_O_CACHE_MASK |BDRV_O_NATIVE_AIO )); else open_flags = flags & ~(BDRV_O_FILE |BDRV_O_SNAPSHOT ); -if (use_bdrv_whitelist && !bdrv_is_whitelisted (drv)) - ret = -ENOTSUP ; -else - ret = drv->bdrv_open (bs, filename, open_flags); + + ret = drv->bdrv_open (bs, filename, open_flags);if ((ret == -EACCES || ret == -EPERM ) && !(flags &BDRV_O_FILE )) { ret = drv->bdrv_open (bs, filename, open_flags & ~BDRV_O_RDWR ); bs->read_only =1 ; } +if (ret <0 ) { -qemu_free (bs->opaque); - bs->opaque =NULL ; - bs->drv =NULL ; - unlink_and_fail: -if (bs->is_temporary) -unlink (filename); -return ret; + goto free_and_fail; } +if (drv->bdrv_getlength) { bs->total_sectors =bdrv_getlength (bs) >>BDRV_SECTOR_BITS ; }
@@ -502,6 +502,15 @@ int bdrv_open2(BlockDriverState *bs, con bs->change_cb (bs->change_opaque); }return 0 ; + +free_and_fail: +qemu_free (bs->opaque); + bs->opaque =NULL ; + bs->drv =NULL ; +unlink_and_fail: +if (bs->is_temporary) +unlink (filename); +return ret; }void bdrv_close (BlockDriverState *bs)
Am 11.01.2010 18:51, schrieb Christoph Hellwig:
Quoted 57 lines
Check the whitelist as early as possible instead of continuing the setup, and move all the error handling code to the end of the function.
Signed-off-by Christoph Hellwig <[email protected]>Index:
qemu/block.c
===================================================================block.c +11
-11@@ -428,10 +428,16 @@ int bdrv_open2(BlockDriverState *bs, con drv =find_image_format (filename); } } +if (!drv) { ret = -ENOENT ; goto unlink_and_fail; } +if (use_bdrv_whitelist && !bdrv_is_whitelisted (drv)) { + ret = -ENOTSUP ; + goto unlink_and_fail; + } + bs->drv = drv; bs->opaque =qemu_mallocz (drv->instance_size);@@ -452,23 +458,17 @@ int bdrv_open2(BlockDriverState *bs, con (flags & (BDRV_O_CACHE_MASK |BDRV_O_NATIVE_AIO )); else open_flags = flags & ~(BDRV_O_FILE |BDRV_O_SNAPSHOT ); -if (use_bdrv_whitelist && !bdrv_is_whitelisted (drv)) - ret = -ENOTSUP ; -else - ret = drv->bdrv_open (bs, filename, open_flags); + + ret = drv->bdrv_open (bs, filename, open_flags);if ((ret == -EACCES || ret == -EPERM ) && !(flags &BDRV_O_FILE )) { ret = drv->bdrv_open (bs, filename, open_flags & ~BDRV_O_RDWR ); bs->read_only =1 ; } +if (ret <0 ) { -qemu_free (bs->opaque); - bs->opaque =NULL ; - bs->drv =NULL ; - unlink_and_fail: -if (bs->is_temporary) -unlink (filename); -return ret; + goto free_and_fail; } +if (drv->bdrv_getlength) { bs->total_sectors =bdrv_getlength (bs) >>BDRV_SECTOR_BITS ; }
block.c int bdrv_open2(BlockDriverState *bs, con · line 510
Quoted 9 lines
block.c +6
-0@@ -502,6 +502,15 @@ int bdrv_open2(BlockDriverState *bs, con bs->change_cb (bs->change_opaque); }return 0 ; + +free_and_fail: +qemu_free (bs->opaque); + bs->opaque =NULL ; + bs->drv =NULL ; +unlink_and_fail:
To keep the behaviour as previously you'd need to set at least bs->drv in the unlink_and_fail case, too. I have no clue why it's important to set it to NULL when bs isn't going to be used anyway (probably it is, but I don't know where), but Fabrice committed this in 6b21b973 as a fix. Unfortunately, the commit message looks like most of Fabrice's commit messages, so it's of no use for me...
Kevin
Quoted 6 lines · Kevin Wolf
To keep the behaviour as previously you'd need to set at least
bs->drvin theunlink_and_failcase, too. I have no clue why it's important to set it to NULL when bs isn't going to be used anyway (probably it is, but I don't know where), but Fabrice committed this in6b21b973as a fix. Unfortunately, the commit message looks like most of Fabrice's commit messages, so it's of no use for me...
The unlink_and_fail label can only be reached from places where we haven't set bs->drv yet, so it should be fine.
Kevin Wolf
Acked-byAm 12.01.2010 18:09, schrieb Christoph Hellwig:
Quoted 10 lines · Kevin Wolf
Quoted 6 lines
To keep the behaviour as previously you'd need to set at least
bs->drvin theunlink_and_failcase, too. I have no clue why it's important to set it to NULL when bs isn't going to be used anyway (probably it is, but I don't know where), but Fabrice committed this in6b21b973as a fix. Unfortunately, the commit message looks like most of Fabrice's commit messages, so it's of no use for me...The
unlink_and_faillabel can only be reached from places where we haven't setbs->drvyet, so it should be fine.
Sorry, I confused drv and bs->drv. Looks alright then.