diff mbox

[4/7] block: simplify bdrv_drop_intermediate

Message ID 1372744789-997-5-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 2, 2013, 5:59 a.m. UTC 
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 71 ++++++++++-------------------------------------------------------
 1 file changed, 11 insertions(+), 60 deletions(-)

Comments

Stefan Hajnoczi July 4, 2013, 2:02 p.m. UTC | #1 
On Tue, Jul 02, 2013 at 01:59:46PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 71 ++++++++++-------------------------------------------------------
>  1 file changed, 11 insertions(+), 60 deletions(-)

Way too big for no commit description.  Okay, I admit I jumped into this
patch and didn't review all the previous ones - maybe it would have been
obvious if I had read them linearly :).

Should the description be something like "We no longer need to a keep a
list because refcounts ..."?
Fam Zheng July 5, 2013, 1 a.m. UTC | #2 
On Thu, 07/04 16:02, Stefan Hajnoczi wrote:
> On Tue, Jul 02, 2013 at 01:59:46PM +0800, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c | 71 ++++++++++-------------------------------------------------------
> >  1 file changed, 11 insertions(+), 60 deletions(-)
> 
> Way too big for no commit description.  Okay, I admit I jumped into this
> patch and didn't review all the previous ones - maybe it would have been
> obvious if I had read them linearly :).
> 
> Should the description be something like "We no longer need to a keep a
> list because refcounts ..."?
> 
Yes, will add a decent message.

Thanks.
diff mbox

Patch

diff --git a/block.c b/block.c
index d1ce570..ae5de17 100644
--- a/block.c
+++ b/block.c
@@ -2015,12 +2015,6 @@  BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
     return overlay;
 }
 
-typedef struct BlkIntermediateStates {
-    BlockDriverState *bs;
-    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
-} BlkIntermediateStates;
-
-
 /*
  * Drops images above 'base' up to and including 'top', and sets the image
  * above 'top' to have base as its backing file.
@@ -2050,15 +2044,9 @@  typedef struct BlkIntermediateStates {
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
                            BlockDriverState *base)
 {
-    BlockDriverState *intermediate;
-    BlockDriverState *base_bs = NULL;
     BlockDriverState *new_top_bs = NULL;
-    BlkIntermediateStates *intermediate_state, *next;
     int ret = -EIO;
 
-    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
-    QSIMPLEQ_INIT(&states_to_delete);
-
     if (!top->drv || !base->drv) {
         goto exit;
     }
@@ -2070,58 +2058,21 @@  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
         goto exit;
     }
 
-    /* special case of new_top_bs->backing_hd already pointing to base - nothing
-     * to do, no intermediate images */
-    if (new_top_bs->backing_hd == base) {
-        ret = 0;
-        goto exit;
-    }
-
-    intermediate = top;
-
-    /* now we will go down through the list, and add each BDS we find
-     * into our deletion queue, until we hit the 'base'
-     */
-    while (intermediate) {
-        intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
-        intermediate_state->bs = intermediate;
-        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
-
-        if (intermediate->backing_hd == base) {
-            base_bs = intermediate->backing_hd;
-            break;
+    while (new_top_bs->backing_hd && new_top_bs->backing_hd != base) {
+        BlockDriverState *backing = new_top_bs->backing_hd;
+        if (backing == NULL) {
+            goto exit;
         }
-        intermediate = intermediate->backing_hd;
-    }
-    if (base_bs == NULL) {
-        /* something went wrong, we did not end at the base. safely
-         * unravel everything, and exit with error */
-        goto exit;
+        new_top_bs->backing_hd = backing->backing_hd;
+        /* break backing_hd chain before releasing bs, so we don't free all the
+         * way up the backing chain */
+        backing->backing_hd = NULL;
+        bdrv_put_ref(backing);
     }
 
-    /* success - we can delete the intermediate states, and link top->base */
-    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
-                                   base_bs->drv ? base_bs->drv->format_name : "");
-    if (ret) {
-        goto exit;
-    }
-    if (new_top_bs->backing_hd) {
-        bdrv_put_ref(new_top_bs->backing_hd);
-    }
-    new_top_bs->backing_hd = base_bs;
-    bdrv_get_ref(base_bs);
-
-    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
-        /* so that bdrv_close() does not recursively close the chain */
-        intermediate_state->bs->backing_hd = NULL;
-        bdrv_put_ref(intermediate_state->bs);
-    }
-    ret = 0;
-
+    ret = bdrv_change_backing_file(new_top_bs, base->filename,
+                                   base->drv ? base->drv->format_name : "");
 exit:
-    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
-        g_free(intermediate_state);
-    }
     return ret;
 }