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 <kw...@redhat.com>
> > Message-Id: <20200507145228.323412-3-kw...@redhat.com>
> > Reviewed-by: Eric Blake <ebl...@redhat.com>
> > Signed-off-by: Kevin Wolf <kw...@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
