On Fri, 30 Apr 2021 at 11:53, Kevin Wolf <[email protected]> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <[email protected]>
>
> Move bdrv_reopen_multiple to new paradigm of permission update:
> first update graph relations, then do refresh the permissions.
>
> We have to modify reopen process in file-posix driver: with new scheme
> we don't have prepared permissions in raw_reopen_prepare(), so we
> should reconfigure fd in raw_check_perm(). Still this seems more native
> and simple anyway.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
> Reviewed-by: Kevin Wolf <[email protected]>
> Message-Id: <[email protected]>
> Signed-off-by: Kevin Wolf <[email protected]>
Hi; Coverity thinks this change introduced a resource leak
(CID 1452772):
> @@ -4271,6 +4270,9 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
> Error **errp)
> {
> int ret = -1;
> BlockReopenQueueEntry *bs_entry, *next;
> + Transaction *tran = tran_new();
> + g_autoptr(GHashTable) found = NULL;
> + g_autoptr(GSList) refresh_list = NULL;
Now we allocate a new Transaction at the start of the function...
>
> assert(bs_queue != NULL);
>
...but in the code between these two hunks there is this:
QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
ret = bdrv_flush(bs_entry->state.bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
goto cleanup;
}
}
which jumps to 'cleanup' on failure...
> - if (ret == 0) {
> - QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
> - BlockDriverState *bs = bs_entry->state.bs;
> + ret = 0;
> + goto cleanup;
>
> - if (bs->drv->bdrv_reopen_commit_post)
> - bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
> +abort:
> + tran_abort(tran);
> + QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> + if (bs_entry->prepared) {
> + bdrv_reopen_abort(&bs_entry->state);
> }
> + qobject_unref(bs_entry->state.explicit_options);
> + qobject_unref(bs_entry->state.options);
> }
> +
> cleanup:
> QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> - if (ret) {
> - if (bs_entry->prepared) {
> - bdrv_reopen_abort(&bs_entry->state);
> - }
> - qobject_unref(bs_entry->state.explicit_options);
> - qobject_unref(bs_entry->state.options);
> - }
> - if (bs_entry->state.new_backing_bs) {
> - bdrv_unref(bs_entry->state.new_backing_bs);
> - }
> g_free(bs_entry);
> }
> g_free(bs_queue);
...and the 'cleanup' label doesn't free the Transaction.
An easy fix would be to move the call to tran_new() down to
below the loop that calls bdrv_flush().
thanks
-- PMM
