From: Denis V. Lunev <[email protected]> The callback runs after bdrv_reopen_multiple() has committed the transaction, so it cannot reject the reopen. It can still find that the node it has just made writable is unusable, and has no way to say so: bdrv_reopen() returns success and the caller carries on.
Give it a return value and an Error argument. The reopen stays committed, the error only reports that the node is gone. Every queued node still gets its callback, the first error is the one reported. A callback may leave its node without a driver, and so may the I/O of a later bdrv_reopen_prepare(), so do not assume that the nodes still ahead of it in the queue have one. qcow2 is the only implementation and does not fail yet. An error therefore means one of two things now, either that the reopen was denied and nothing changed, or that it went through and left a tree which cannot be used. Nothing is undone in the second case: the node is beyond repair by another reopen, and a caller which reacts to the error by reopening anything is making it worse. bdrv_reopen_multiple() says so, nothing else changes. Signed-off-by: Denis V. Lunev <[email protected]> CC: Kevin Wolf <[email protected]> CC: Hanna Reitz <[email protected]> CC: Andrey Drobyshev <[email protected]> --- block.c | 24 +++++++++++++++++++++--- block/qcow2.c | 4 +++- include/block/block_int-common.h | 9 +++++++-- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/block.c b/block.c index e39f15816a..b29202c8d5 100644 --- a/block.c +++ b/block.c @@ -4582,6 +4582,10 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue) * If all devices prepare successfully, then the changes are committed * to all devices. * + * A failure means either that the reopen was denied and nothing changed, + * or that it went through and a driver then found the node unusable. In + * the second case nothing is undone and the tree is no longer usable. + * * All affected nodes must be drained between bdrv_reopen_queue() and * bdrv_reopen_multiple(). * @@ -4658,15 +4662,29 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) tran_commit(tran); bdrv_graph_wrunlock(); + ret = 0; QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) { BlockDriverState *bs = bs_entry->state.bs; + Error *local_err = NULL; + int commit_ret; - if (bs->drv->bdrv_reopen_commit_post) { - bs->drv->bdrv_reopen_commit_post(&bs_entry->state); + if (!bs->drv || !bs->drv->bdrv_reopen_commit_post) { + continue; + } + + commit_ret = bs->drv->bdrv_reopen_commit_post(&bs_entry->state, + &local_err); + assert(commit_ret >= 0 || local_err); + + if (commit_ret < 0 && ret == 0) { + /* Committed already, so report the first failure and go on */ + error_propagate(errp, local_err); + ret = commit_ret; + } else { + error_free(local_err); } } - ret = 0; goto cleanup; abort: diff --git a/block/qcow2.c b/block/qcow2.c index 1543255eba..553a94d003 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2145,7 +2145,7 @@ static void qcow2_reopen_commit(BDRVReopenState *state) g_free(state->opaque); } -static void qcow2_reopen_commit_post(BDRVReopenState *state) +static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp) { GRAPH_RDLOCK_GUARD_MAINLOOP(); @@ -2163,6 +2163,8 @@ static void qcow2_reopen_commit_post(BDRVReopenState *state) bdrv_get_node_name(state->bs)); } } + + return 0; } static void qcow2_reopen_abort(BDRVReopenState *state) diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 147c08155f..035e54d434 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -239,8 +239,13 @@ struct BlockDriver { BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp); void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)( BDRVReopenState *reopen_state); - void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)( - BDRVReopenState *reopen_state); + /* + * Runs once the reopen is committed, so it cannot reject it. Returns 0, + * or a negative errno with @errp set to report that the node it has + * just reopened is unusable, which it may leave without a driver. + */ + int GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)( + BDRVReopenState *reopen_state, Error **errp); void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)( BDRVReopenState *reopen_state); void (*bdrv_join_options)(QDict *options, QDict *old_options); -- 2.53.0
