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

[Vladimir Sementsov-Ogievskiy]

12.05.2020 21:48, Kevin Wolf wrote:
> 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 <address@hidden>
> > > Message-Id: <address@hidden>
> > > Reviewed-by: Eric Blake <address@hidden>
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >    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.

Hmm. but I failed to find, where job->err is reported except for 
job_query_single(), which doesn't call job_update_rc().

block_job_event_completed() doesn't use job->err, but instead create a message 
using strerror(-job->job.ret).

Interesting also, that job_finish_sync may return error, not setting errp.. 
Still except for tests, it should influence only run_block_job() from qemu-img, 
which itself doesn't care too much about setting errp on failure, so it's 
broken anyway.

OK, seems this all is not very related to the series:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


--
Best regards,
Vladimir