Commit 5fe31c25 authored by Kevin Wolf's avatar Kevin Wolf
Browse files

block: Fix error handling in bdrv_replace_in_backing_chain() 



When adding an Error parameter, bdrv_replace_in_backing_chain() would
become nothing more than a wrapper around change_parent_backing_link().
So make the latter public, renamed as bdrv_replace_node(), and remove
bdrv_replace_in_backing_chain().

Most of the callers just remove a node from the graph that they just
inserted, so they can use &error_abort, but completion of a mirror job
with 'replaces' set can actually fail.

Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
Reviewed-by: default avatarFam Zheng <famz@redhat.com>
Reviewed-by: default avatarEric Blake <eblake@redhat.com>
parent 234ac1a9

block.c

+6 −19
Original line number Diff line number Diff line
@@ -2932,8 +2932,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) 
    return true;

}



static void change_parent_backing_link(BlockDriverState *from,

                                       BlockDriverState *to, Error **errp)

void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,

                       Error **errp)

{

    BdrvChild *c, *next;

    GSList *list = NULL, *p;
@@ -2941,6 +2941,9 @@ static void change_parent_backing_link(BlockDriverState *from, 
    uint64_t perm = 0, shared = BLK_PERM_ALL;

    int ret;



    assert(!atomic_read(&from->in_flight));

    assert(!atomic_read(&to->in_flight));



    /* Make sure that @from doesn't go away until we have successfully attached

     * all of its parents to @to. */

    bdrv_ref(from);
@@ -3003,16 +3006,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, 
{

    Error *local_err = NULL;



    assert(!atomic_read(&bs_top->in_flight));

    assert(!atomic_read(&bs_new->in_flight));



    bdrv_set_backing_hd(bs_new, bs_top, &local_err);

    if (local_err) {

        error_propagate(errp, local_err);

        goto out;

    }



    change_parent_backing_link(bs_top, bs_new, &local_err);

    bdrv_replace_node(bs_top, bs_new, &local_err);

    if (local_err) {

        error_propagate(errp, local_err);

        bdrv_set_backing_hd(bs_new, NULL, &error_abort);
@@ -3025,19 +3025,6 @@ out: 
    bdrv_unref(bs_new);

}



void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)

{

    assert(!bdrv_requests_pending(old));

    assert(!bdrv_requests_pending(new));



    bdrv_ref(old);



    /* FIXME Proper error handling */

    change_parent_backing_link(old, new, &error_abort);



    bdrv_unref(old);

}



static void bdrv_delete(BlockDriverState *bs)

{

    assert(!bs->job);

block/mirror.c

+9 −6
Original line number Diff line number Diff line
@@ -550,8 +550,12 @@ static void mirror_exit(BlockJob *job, void *opaque) 
        /* The mirror job has no requests in flight any more, but we need to

         * drain potential other users of the BDS before changing the graph. */

        bdrv_drained_begin(target_bs);

        bdrv_replace_in_backing_chain(to_replace, target_bs);

        bdrv_replace_node(to_replace, target_bs, &local_err);

        bdrv_drained_end(target_bs);

        if (local_err) {

            error_report_err(local_err);

            data->ret = -EPERM;

        }

    }

    if (s->to_replace) {

        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
@@ -570,12 +574,11 @@ static void mirror_exit(BlockJob *job, void *opaque) 
     * block the removal. */

    block_job_remove_all_bdrv(job);

    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);

    bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));

    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);



    /* We just changed the BDS the job BB refers to (with either or both of the

     * bdrv_replace_in_backing_chain() calls), so switch the BB back so the

     * cleanup does the right thing. We don't need any permissions any more

     * now. */

     * bdrv_replace_node() calls), so switch the BB back so the cleanup does

     * the right thing. We don't need any permissions any more now. */

    blk_remove_bs(job->blk);

    blk_set_perm(job->blk, 0, BLK_PERM_ALL, &error_abort);

    blk_insert_bs(job->blk, mirror_top_bs, &error_abort);
@@ -1234,7 +1237,7 @@ fail: 
    }



    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);

    bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));

    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);

}



void mirror_start(const char *job_id, BlockDriverState *bs,

blockdev.c

+1 −1
Original line number Diff line number Diff line
@@ -1806,7 +1806,7 @@ static void external_snapshot_abort(BlkActionState *common) 
                             DO_UPCAST(ExternalSnapshotState, common, common);

    if (state->new_bs) {

        if (state->overlay_appended) {

            bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);

            bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);

        }

    }

}

include/block/block.h

+2 −2
Original line number Diff line number Diff line
@@ -238,8 +238,8 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); 
BlockDriverState *bdrv_new(void);

void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,

                 Error **errp);

void bdrv_replace_in_backing_chain(BlockDriverState *old,

                                   BlockDriverState *new);

void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,

                       Error **errp);



int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);

int bdrv_parse_discard_flags(const char *mode, int *flags);

include/block/block_int.h

+2 −2
Original line number Diff line number Diff line
@@ -441,8 +441,8 @@ typedef struct BdrvAioNotifier { 
} BdrvAioNotifier;



struct BdrvChildRole {

    /* If true, bdrv_replace_in_backing_chain() doesn't change the node this

     * BdrvChild points to. */

    /* If true, bdrv_replace_node() doesn't change the node this BdrvChild

     * points to. */

    bool stay_at_node;



    void (*inherit_options)(int *child_flags, QDict *child_options,