[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2] blockjob: update nodes head while removing a
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2] blockjob: update nodes head while removing all bdrv |
Date: |
Tue, 10 Sep 2019 15:56:42 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 10.09.19 15:36, Sergio Lopez wrote:
> block_job_remove_all_bdrv() iterates through job->nodes, calling
> bdrv_root_unref_child() for each entry. The call to the latter may
> reach child_job_[can_]set_aio_ctx(), which will also attempt to
> traverse job->nodes, potentially finding entries that where freed
> on previous iterations.
>
> To avoid this situation, update job->nodes head on each iteration to
> ensure that already freed entries are no longer linked to the list.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1746631
> Signed-off-by: Sergio Lopez <address@hidden>
> ---
> Changelog
>
> v2:
> - Avoid leaking job->nodes (thanks Max Reitz)
> ---
> blockjob.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
> ---
> diff --git a/blockjob.c b/blockjob.c
> index 6e32d1a0c0..ffda6dd1e4 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -187,13 +187,21 @@ static const BdrvChildRole child_job = {
>
> void block_job_remove_all_bdrv(BlockJob *job)
> {
> - GSList *l;
> + GSList *l, *orig_nodes;
> +
> + orig_nodes = job->nodes;
> for (l = job->nodes; l; l = l->next) {
> BdrvChild *c = l->data;
> bdrv_op_unblock_all(c->bs, job->blocker);
> bdrv_root_unref_child(c);
> + /*
> + * The call above may reach child_job_[can_]set_aio_ctx(), which will
> + * also traverse job->nodes, so update the head here to make sure it
> + * doesn't attempt to process an already freed BdrvChild.
> + */
> + job->nodes = l->next;
> }
> - g_slist_free(job->nodes);
> + g_slist_free(orig_nodes);
> job->nodes = NULL;
Hm, this assignment is now a no-op.
I think I’d just rewrite the whole function in the following fashion:
orig_nodes = job->nodes;
while (job->nodes) {
BdrvChild *c = job->nodes->data;
[...]
job->nodes = job->nodes->next;
}
g_slist_free(orig_nodes);
What do you think?
> }
>
>
signature.asc
Description: OpenPGP digital signature