Re: [Qemu-devel] [PATCH 1/2] block: clean up bdrv_open2 structure a bit

2010-01-12 Thread Kevin Wolf 
Am 11.01.2010 18:51, schrieb Christoph Hellwig:
 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 h...@lst.de
 
 Index: qemu/block.c
 ===
 --- qemu.orig/block.c 2010-01-11 17:48:27.811004054 +0100
 +++ qemu/block.c  2010-01-11 17:52:15.578006158 +0100
 @@ -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:

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

Re: [Qemu-devel] [PATCH 1/2] block: clean up bdrv_open2 structure a bit

2010-01-12 Thread Kevin Wolf 
Am 12.01.2010 18:09, schrieb Christoph Hellwig:
 On Tue, Jan 12, 2010 at 05:04:00PM +0100, Kevin Wolf wrote:
 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...
 
 The unlink_and_fail label can only be reached from places where we
 haven't set bs-drv yet, so it should be fine.

Sorry, I confused drv and bs-drv. Looks alright then.

Acked-by: Kevin Wolf kw...@redhat.com

[Qemu-devel] [PATCH 1/2] block: clean up bdrv_open2 structure a bit

2010-01-11 Thread Christoph Hellwig 
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 h...@lst.de

Index: qemu/block.c
===
--- qemu.orig/block.c   2010-01-11 17:48:27.811004054 +0100
+++ qemu/block.c2010-01-11 17:52:15.578006158 +0100
@@ -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)