Re: [PATCH v2 3/4] mirror: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

11.05.2020 16:58, Kevin Wolf wrote:
> If the target is shorter than the source, mirror would copy data until
> it reaches the end of the target and then fail with an I/O error when
> trying to write past the end.
> 
> If the target is longer than the source, the mirror job would complete
> successfully, but the target wouldn't actually be an accurate copy of
> the source image (it would contain some additional garbage at the end).
> 
> Fix this by checking that both images have the same size when the job
> starts.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20200507145228.323412-3-kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/mirror.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index aca95c9bc9..201ffa26f9 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -872,6 +872,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>       BlockDriverState *target_bs = blk_bs(s->target);
>       bool need_drain = true;
>       int64_t length;
> +    int64_t target_length;
>       BlockDriverInfo bdi;
>       char backing_filename[2]; /* we only need 2 characters because we are only
>                                    checking for a NULL string */
> @@ -887,24 +888,26 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>           goto immediate_exit;
>       }
>   
> +    target_length = blk_getlength(s->target);
> +    if (target_length < 0) {
> +        ret = target_length;
> +        goto immediate_exit;
> +    }
> +
>       /* Active commit must resize the base image if its size differs from the
>        * active layer. */
>       if (s->base == blk_bs(s->target)) {
> -        int64_t base_length;
> -
> -        base_length = blk_getlength(s->target);
> -        if (base_length < 0) {
> -            ret = base_length;
> -            goto immediate_exit;
> -        }
> -
> -        if (s->bdev_length > base_length) {
> +        if (s->bdev_length > target_length) {
>               ret = blk_truncate(s->target, s->bdev_length, false,
>                                  PREALLOC_MODE_OFF, 0, NULL);
>               if (ret < 0) {
>                   goto immediate_exit;
>               }
>           }

Hmm, interesting, if base is larger, is our behavior correct? Blockdev becomes larger after commit and old data becomes available? I think we should zero the tail after old EOF or shrink the target..

> +    } else if (s->bdev_length != target_length) {
> +        error_setg(errp, "Source and target image have different sizes");
> +        ret = -EINVAL;

Seems, the only case, when mirror_run() sets errp. And, therefore, the only correct one..

> +        goto immediate_exit;
>       }
>   
>       if (s->bdev_length == 0) {
> 


-- 
Best regards,
Vladimir