Re: [Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies

[Max Reitz <mreitz@redhat.com>]

On 12.09.2014 21:26, Markus Armbruster wrote:
> If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
> the BDS.  This can happen in three places:
>
> * Device model destruction during unplug: blockdev_auto_del()
>
> * Xen IDE unplug: pci_piix3_xen_ide_unplug()
>
> * drive_del command when no device model is attached: do_drive_del()
>
> The other callers of drive_del are on error paths where refcnt == 1.
>
> If the user somehow manages to plug in a device model using a BDS that
> has gone through drive_del(), the legacy configuration passed in
> DriveInfo doesn't reach the device model, and automatic deletion on
> unplug doesn't work.  Worse, some device models such as scsi-disk
> crash when DriveInfo doesn't exist.
>
> This is theoretical; I didn't research an actual reproducer.
>
> Fix by keeping DriveInfo alive until its BDS dies.
>
> This affects qemu_drive_opts: now you can't reuse the same ID for new
> drive options until the BDS dies.  Before, you could, but since the
> code always attempts to create a BDS with the same ID next, the
> enclosing operation "create a new drive" failed anyway.  Different
> error path, same result.
>
> Unfortunately, the fix involves use of blockdev.c stuff from block.c,
> which is a layering violation.  Fortunately, my forthcoming
> BlockBackend work will get rid of it again.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block.c                   |  2 ++
>   blockdev.c                | 13 ++++++++-----
>   include/sysemu/blockdev.h |  1 +
>   stubs/Makefile.objs       |  1 +
>   stubs/blockdev.c          | 12 ++++++++++++
>   5 files changed, 24 insertions(+), 5 deletions(-)
>   create mode 100644 stubs/blockdev.c

Seems reasonable.

Reviewed-by: Max Reitz <mreitz@redhat.com>