Re: [Qemu-devel] [PATCH v2 03/21] mirror: Error out when a BDS would get two BBs

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3324 bytes --]

On 23.11.2015 16:59, Kevin Wolf wrote:
> bdrv_replace_in_backing_chain() asserts that not both old and new
> BlockDdriverState have a BlockBackend attached to them because both
> would have to end up pointing to the new BDS and we don't support more
> than one BB per BDS yet.
> 
> Before we can safely allow references to existing nodes as backing
> files, we need to make sure that even if a backing file has a BB on it,
> this doesn't crash qemu.
> 
> There are probably also some cases with the 'replaces' option set where
> drive-mirror could fail this assertion today. They are fixed with this
> error check as well.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 52c9abf..0620068 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -18,6 +18,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/error-report.h"
>  
>  #define SLICE_TIME    100000000ULL /* ns */
>  #define MAX_IN_FLIGHT 16
> @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
>          if (s->to_replace) {
>              to_replace = s->to_replace;
>          }
> +
> +        /* This was checked in mirror_start_job(), but meanwhile one of the
> +         * nodes could have been newly attached to a BlockBackend. */
> +        if (to_replace->blk && s->target->blk) {
> +            error_report("block job: Can't create node with two BlockBackends");
> +            data->ret = -EINVAL;
> +            goto out;
> +        }
> +
>          if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
>              bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
>          }
>          bdrv_replace_in_backing_chain(to_replace, s->target);
>      }
> +
> +out:
>      if (s->to_replace) {
>          bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
>          error_free(s->replace_blocker);
> @@ -701,6 +713,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>                               bool is_none_mode, BlockDriverState *base)
>  {
>      MirrorBlockJob *s;
> +    BlockDriverState *replaced_bs;
>  
>      if (granularity == 0) {
>          granularity = bdrv_get_default_bitmap_granularity(target);
> @@ -724,6 +737,21 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>          buf_size = DEFAULT_MIRROR_BUF_SIZE;
>      }
>  
> +    /* We can't support this case as long as the block layer can't handle
> +     * multple BlockBackends per BlockDriverState. */

*multiple

With that fixed:

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

> +    if (replaces) {
> +        replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
> +        if (replaced_bs == NULL) {
> +            return;
> +        }
> +    } else {
> +        replaced_bs = bs;
> +    }
> +    if (replaced_bs->blk && target->blk) {
> +        error_setg(errp, "Can't create node with two BlockBackends");
> +        return;
> +    }
> +
>      s = block_job_create(driver, bs, speed, cb, opaque, errp);
>      if (!s) {
>          return;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]