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

[Kevin Wolf <kwolf@redhat.com>]

Am 12.05.2020 um 19:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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..

Yes, I agree, we should shrink it. But active commit is a different case
than what I'm fixing in this patch, so I left it unmodified. We'll
probably need a third series for commit after backup and mirror.

> > +    } 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..

job_update_rc() takes care to fill job->err (with strerror()) if it
hasn't been set yet, so the other places aren't strictly wrong, but
probably provide suboptimal error messages.

Kevin