Re: [Qemu-devel] [PATCH v8 08/12] block: Parse backing option to reference existing BDS

2014-01-07 Thread Fam Zheng 

On 2014年01月03日 17:19, Stefan Hajnoczi wrote:

  }

@@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
  assert(QLIST_EMPTY(bs_new-dirty_bitmaps));
  assert(bs_new-job == NULL);
  assert(bs_new-dev == NULL);
-assert(bdrv_op_blocker_is_empty(bs_new));
  assert(bs_new-io_limits_enabled == false);
  assert(!throttle_have_timer(bs_new-throttle_state));

@@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
  /* Check a few fields that should remain attached to the device */
  assert(bs_new-dev == NULL);
  assert(bs_new-job == NULL);
-assert(bdrv_op_blocker_is_empty(bs_new));
  assert(bs_new-io_limits_enabled == false);
  assert(!throttle_have_timer(bs_new-throttle_state));


Why are these hunks part of this patch?  I guess it makes sense *not* to
check for blockers in bdrv_swap().  Instead the high-level functions in
blockdev.c and elsewhere should check blockers.



The two checks are here because of the mechanical replace of in_use. 
They are removed because it is no longer true for some valid cases, e.g 
with block-commit. So we need these hunks here, or do this as a 
preceding in the series.


Will update the commit message and keep it as is.

Fam

Re: [Qemu-devel] [PATCH v8 08/12] block: Parse backing option to reference existing BDS

2014-01-03 Thread Stefan Hajnoczi 
On Fri, Dec 13, 2013 at 03:35:16PM +0800, Fam Zheng wrote:
 diff --git a/block.c b/block.c
 index b3993d7..fba7148 100644
 --- a/block.c
 +++ b/block.c
 @@ -1191,11 +1191,25 @@ int bdrv_open(BlockDriverState *bs, const char 
 *filename, QDict *options,
  /* If there is a backing file, use it */
  if ((flags  BDRV_O_NO_BACKING) == 0) {
  QDict *backing_options;
 -
 -qdict_extract_subqdict(options, backing_options, backing.);
 -ret = bdrv_open_backing_file(bs, backing_options, local_err);
 -if (ret  0) {
 -goto close_and_fail;
 +const char *backing_name;
 +BlockDriverState *backing_hd;
 +
 +backing_name = qdict_get_try_str(options, backing);
 +qdict_del(options, backing);

This causes a use-after-free since backing_name is a const char pointer
to the qdict element!

 +if (backing_name) {
 +backing_hd = bdrv_find(backing_name);
 +if (!backing_hd) {
 +error_set(local_err, QERR_DEVICE_NOT_FOUND, backing_name);
 +ret = -ENOENT;
 +goto close_and_fail;
 +}
 +bdrv_set_backing_hd(bs, backing_hd);
 +} else {
 +qdict_extract_subqdict(options, backing_options, backing.);
 +ret = bdrv_open_backing_file(bs, backing_options, local_err);
 +if (ret  0) {
 +goto close_and_fail;
 +}
  }

Seems like users can specify backing=foo backing.file=/tmp/a and we
ignore backing.file.  Is it useful to silently ignore the backing.
subdict?  The user may have given useless options by mistake.  An error
would help prevent weird options combinations.

  }
  
 @@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, 
 BlockDriverState *bs_old)
  assert(QLIST_EMPTY(bs_new-dirty_bitmaps));
  assert(bs_new-job == NULL);
  assert(bs_new-dev == NULL);
 -assert(bdrv_op_blocker_is_empty(bs_new));
  assert(bs_new-io_limits_enabled == false);
  assert(!throttle_have_timer(bs_new-throttle_state));
  
 @@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, 
 BlockDriverState *bs_old)
  /* Check a few fields that should remain attached to the device */
  assert(bs_new-dev == NULL);
  assert(bs_new-job == NULL);
 -assert(bdrv_op_blocker_is_empty(bs_new));
  assert(bs_new-io_limits_enabled == false);
  assert(!throttle_have_timer(bs_new-throttle_state));

Why are these hunks part of this patch?  I guess it makes sense *not* to
check for blockers in bdrv_swap().  Instead the high-level functions in
blockdev.c and elsewhere should check blockers.

[Qemu-devel] [PATCH v8 08/12] block: Parse backing option to reference existing BDS

2013-12-13 Thread Fam Zheng 
Now it's safe to allow reference for backing_hd in the interface.

Signed-off-by: Fam Zheng f...@redhat.com
---
 block.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index b3993d7..fba7148 100644
--- a/block.c
+++ b/block.c
@@ -1191,11 +1191,25 @@ int bdrv_open(BlockDriverState *bs, const char 
*filename, QDict *options,
 /* If there is a backing file, use it */
 if ((flags  BDRV_O_NO_BACKING) == 0) {
 QDict *backing_options;
-
-qdict_extract_subqdict(options, backing_options, backing.);
-ret = bdrv_open_backing_file(bs, backing_options, local_err);
-if (ret  0) {
-goto close_and_fail;
+const char *backing_name;
+BlockDriverState *backing_hd;
+
+backing_name = qdict_get_try_str(options, backing);
+qdict_del(options, backing);
+if (backing_name) {
+backing_hd = bdrv_find(backing_name);
+if (!backing_hd) {
+error_set(local_err, QERR_DEVICE_NOT_FOUND, backing_name);
+ret = -ENOENT;
+goto close_and_fail;
+}
+bdrv_set_backing_hd(bs, backing_hd);
+} else {
+qdict_extract_subqdict(options, backing_options, backing.);
+ret = bdrv_open_backing_file(bs, backing_options, local_err);
+if (ret  0) {
+goto close_and_fail;
+}
 }
 }
 
@@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
 assert(QLIST_EMPTY(bs_new-dirty_bitmaps));
 assert(bs_new-job == NULL);
 assert(bs_new-dev == NULL);
-assert(bdrv_op_blocker_is_empty(bs_new));
 assert(bs_new-io_limits_enabled == false);
 assert(!throttle_have_timer(bs_new-throttle_state));
 
@@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
 /* Check a few fields that should remain attached to the device */
 assert(bs_new-dev == NULL);
 assert(bs_new-job == NULL);
-assert(bdrv_op_blocker_is_empty(bs_new));
 assert(bs_new-io_limits_enabled == false);
 assert(!throttle_have_timer(bs_new-throttle_state));
 
-- 
1.8.5.1