| Message ID | 20171006155422.10135-35-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/54] block: Typo fix in copy_on_readv() | expand |
On Fri, 6 Oct 2017 at 17:20, Kevin Wolf <kwolf@redhat.com> wrote: > > This changes the commit block job to support operation in a graph where > there is more than a single active layer that references the top node. > > This involves inserting the commit filter node not only on the path > between the given active node and the top node, but between the top node > and all of its parents. > > On completion, bdrv_drop_intermediate() must consider all parents for > updating the backing file link. These parents may be backing files > themselves and as such read-only; reopen them temporarily if necessary. > Previously this was achieved by the bdrv_reopen() calls in the commit > block job that made overlay_bs read-write for the whole duration of the > block job, even though write access is only needed on completion. > > Now that we consider all parents, overlay_bs is meaningless. It is left > in place in this commit, but we'll remove it soon. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Hi -- a recent change to the block layer has caused Coverity to flag up a possible issue with this older commit: CID 1399710: > @@ -3492,42 +3504,42 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, > goto exit; > } > > - new_top_bs = bdrv_find_overlay(active, top); > - > - if (new_top_bs == NULL) { > - /* we could not find the image above 'top', this is an error */ > - goto exit; > - } > - > - /* special case of new_top_bs->backing->bs already pointing to base - nothing > - * to do, no intermediate images */ > - if (backing_bs(new_top_bs) == base) { > - ret = 0; > - goto exit; > - } > - > /* Make sure that base is in the backing chain of top */ > if (!bdrv_chain_contains(top, base)) { > goto exit; > } > > /* success - we can delete the intermediate states, and link top->base */ > - if (new_top_bs->backing->role->update_filename) { > - backing_file_str = backing_file_str ? backing_file_str : base->filename; > - ret = new_top_bs->backing->role->update_filename(new_top_bs->backing, > - base, backing_file_str, > - &local_err); > - if (ret < 0) { > - bdrv_set_backing_hd(new_top_bs, top, &error_abort); > + backing_file_str = backing_file_str ? backing_file_str : base->filename; > + > + QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) { > + /* Check whether we are allowed to switch c from top to base */ > + GSList *ignore_children = g_slist_prepend(NULL, c); > + bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm, > + ignore_children, &local_err); Here we don't check the return value from bdrv_check_update_perm(), which we do in all four other places that we call it. I think this is probably a false positive in that the function will only return ret < 0 if it also returns a failure via local_err, but it might be worth being consistent just to placate Coverity. > + if (local_err) { > + ret = -EPERM; > + error_report_err(local_err); > goto exit; > } > - } Happy to just mark the issue as a false-positive in the Coverity UI if you think that's the best resolution. thanks -- PMM
Am 14.03.2019 um 18:06 hat Peter Maydell geschrieben: > On Fri, 6 Oct 2017 at 17:20, Kevin Wolf <kwolf@redhat.com> wrote: > > > > This changes the commit block job to support operation in a graph where > > there is more than a single active layer that references the top node. > > > > This involves inserting the commit filter node not only on the path > > between the given active node and the top node, but between the top node > > and all of its parents. > > > > On completion, bdrv_drop_intermediate() must consider all parents for > > updating the backing file link. These parents may be backing files > > themselves and as such read-only; reopen them temporarily if necessary. > > Previously this was achieved by the bdrv_reopen() calls in the commit > > block job that made overlay_bs read-write for the whole duration of the > > block job, even though write access is only needed on completion. > > > > Now that we consider all parents, overlay_bs is meaningless. It is left > > in place in this commit, but we'll remove it soon. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Hi -- a recent change to the block layer has caused Coverity > to flag up a possible issue with this older commit: CID 1399710: > > > @@ -3492,42 +3504,42 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, > > goto exit; > > } > > > > - new_top_bs = bdrv_find_overlay(active, top); > > - > > - if (new_top_bs == NULL) { > > - /* we could not find the image above 'top', this is an error */ > > - goto exit; > > - } > > - > > - /* special case of new_top_bs->backing->bs already pointing to base - nothing > > - * to do, no intermediate images */ > > - if (backing_bs(new_top_bs) == base) { > > - ret = 0; > > - goto exit; > > - } > > - > > /* Make sure that base is in the backing chain of top */ > > if (!bdrv_chain_contains(top, base)) { > > goto exit; > > } > > > > /* success - we can delete the intermediate states, and link top->base */ > > - if (new_top_bs->backing->role->update_filename) { > > - backing_file_str = backing_file_str ? backing_file_str : base->filename; > > - ret = new_top_bs->backing->role->update_filename(new_top_bs->backing, > > - base, backing_file_str, > > - &local_err); > > - if (ret < 0) { > > - bdrv_set_backing_hd(new_top_bs, top, &error_abort); > > + backing_file_str = backing_file_str ? backing_file_str : base->filename; > > + > > + QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) { > > + /* Check whether we are allowed to switch c from top to base */ > > + GSList *ignore_children = g_slist_prepend(NULL, c); > > + bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm, > > + ignore_children, &local_err); > > Here we don't check the return value from bdrv_check_update_perm(), > which we do in all four other places that we call it. I think this > is probably a false positive in that the function will only > return ret < 0 if it also returns a failure via local_err, but > it might be worth being consistent just to placate Coverity. I agree, this shouldn't be a problem in theory. I've sent a patch anyway to make things more consistent. Kevin > > + if (local_err) { > > + ret = -EPERM; > > + error_report_err(local_err); > > goto exit; > > } > > - } > > Happy to just mark the issue as a false-positive in the Coverity > UI if you think that's the best resolution. > > thanks > -- PMM
diff --git a/block.c b/block.c index ab354ba82a..1b098d4d09 100644 --- a/block.c +++ b/block.c @@ -985,14 +985,26 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base, const char *filename, Error **errp) { BlockDriverState *parent = c->opaque; + int orig_flags = bdrv_get_flags(parent); int ret; + if (!(orig_flags & BDRV_O_RDWR)) { + ret = bdrv_reopen(parent, orig_flags | BDRV_O_RDWR, errp); + if (ret < 0) { + return ret; + } + } + ret = bdrv_change_backing_file(parent, filename, base->drv ? base->drv->format_name : ""); if (ret < 0) { error_setg_errno(errp, ret, "Could not update backing file link"); } + if (!(orig_flags & BDRV_O_RDWR)) { + bdrv_reopen(parent, orig_flags, NULL); + } + return ret; } @@ -3482,7 +3494,7 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs) int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, BlockDriverState *base, const char *backing_file_str) { - BlockDriverState *new_top_bs = NULL; + BdrvChild *c, *next; Error *local_err = NULL; int ret = -EIO; @@ -3492,42 +3504,42 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, goto exit; } - new_top_bs = bdrv_find_overlay(active, top); - - if (new_top_bs == NULL) { - /* we could not find the image above 'top', this is an error */ - goto exit; - } - - /* special case of new_top_bs->backing->bs already pointing to base - nothing - * to do, no intermediate images */ - if (backing_bs(new_top_bs) == base) { - ret = 0; - goto exit; - } - /* Make sure that base is in the backing chain of top */ if (!bdrv_chain_contains(top, base)) { goto exit; } /* success - we can delete the intermediate states, and link top->base */ - if (new_top_bs->backing->role->update_filename) { - backing_file_str = backing_file_str ? backing_file_str : base->filename; - ret = new_top_bs->backing->role->update_filename(new_top_bs->backing, - base, backing_file_str, - &local_err); - if (ret < 0) { - bdrv_set_backing_hd(new_top_bs, top, &error_abort); + backing_file_str = backing_file_str ? backing_file_str : base->filename; + + QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) { + /* Check whether we are allowed to switch c from top to base */ + GSList *ignore_children = g_slist_prepend(NULL, c); + bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm, + ignore_children, &local_err); + if (local_err) { + ret = -EPERM; + error_report_err(local_err); goto exit; } - } + g_slist_free(ignore_children); - bdrv_set_backing_hd(new_top_bs, base, &local_err); - if (local_err) { - ret = -EPERM; - error_report_err(local_err); - goto exit; + /* If so, update the backing file path in the image file */ + if (c->role->update_filename) { + ret = c->role->update_filename(c, base, backing_file_str, + &local_err); + if (ret < 0) { + bdrv_abort_perm_update(base); + error_report_err(local_err); + goto exit; + } + } + + /* Do the actual switch in the in-memory graph. + * Completes bdrv_check_update_perm() transaction internally. */ + bdrv_ref(base); + bdrv_replace_child(c, base); + bdrv_unref(top); } ret = 0; diff --git a/block/commit.c b/block/commit.c index 8f0e83578a..610e1cd8f5 100644 --- a/block/commit.c +++ b/block/commit.c @@ -350,7 +350,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, error_propagate(errp, local_err); goto fail; } - bdrv_set_backing_hd(overlay_bs, commit_top_bs, &local_err); + bdrv_replace_node(top, commit_top_bs, &local_err); if (local_err) { bdrv_unref(commit_top_bs); commit_top_bs = NULL;
This changes the commit block job to support operation in a graph where there is more than a single active layer that references the top node. This involves inserting the commit filter node not only on the path between the given active node and the top node, but between the top node and all of its parents. On completion, bdrv_drop_intermediate() must consider all parents for updating the backing file link. These parents may be backing files themselves and as such read-only; reopen them temporarily if necessary. Previously this was achieved by the bdrv_reopen() calls in the commit block job that made overlay_bs read-write for the whole duration of the block job, even though write access is only needed on completion. Now that we consider all parents, overlay_bs is meaningless. It is left in place in this commit, but we'll remove it soon. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 68 ++++++++++++++++++++++++++++++++++------------------------ block/commit.c | 2 +- 2 files changed, 41 insertions(+), 29 deletions(-)