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

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

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

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 <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir