Re: [Qemu-devel] [PATCH 08/34] block: Add list of children to BlockDriverState

[Kevin Wolf <kwolf@redhat.com>]

Am 08.05.2015 um 19:21 hat Kevin Wolf geschrieben:
> This allows iterating over all children of a given BDS, not only
> including bs->file and bs->backing_hd, but also driver-specific
> ones like VMDK extents or Quorum children.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 27 +++++++++++++++++++++++++++
>  include/block/block_int.h |  8 ++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/block.c b/block.c
> index c4f0fb4..59f54ed 100644
> --- a/block.c
> +++ b/block.c
> @@ -1301,6 +1301,19 @@ out:
>      return ret;
>  }
>  
> +static void bdrv_attach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs,
> +                              const BdrvChildRole *child_role)
> +{
> +    BdrvChild *child = g_new(BdrvChild, 1);
> +    *child = (BdrvChild) {
> +        .bs     = child_bs,
> +        .role   = child_role,
> +    };
> +
> +    QLIST_INSERT_HEAD(&parent_bs->children, child, next);
> +}
> +
>  /*
>   * Opens a disk image (raw, qcow2, vmdk, ...)
>   *
> @@ -1353,6 +1366,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>              return -ENODEV;
>          }
>          bdrv_ref(bs);
> +        if (child_role) {
> +            bdrv_attach_child(parent, bs, child_role);
> +        }
>          *pbs = bs;
>          return 0;
>      }
> @@ -1495,6 +1511,10 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>          goto close_and_fail;
>      }
>  
> +    if (child_role) {
> +        bdrv_attach_child(parent, bs, child_role);
> +    }
> +
>      QDECREF(options);
>      *pbs = bs;
>      return 0;
> @@ -1789,6 +1809,12 @@ void bdrv_close(BlockDriverState *bs)
>      notifier_list_notify(&bs->close_notifiers, bs);
>  
>      if (bs->drv) {
> +        BdrvChild *child, *next;
> +
> +        QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
> +            g_free(child);
> +        }

Max already pointed out this place, but we both didn't see the real bug
here: Without a QLIST_REMOVE(), we get use after free on the next open
of this BDS.

After the latest rebase, the floppy media change qtest ended up failing
because of this. Who said that time invested in fdc is wasted? ;-)

Kevin