| Message ID | 20200511135825.219437-4-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mirror: Make sure that source and target size match | expand |
On 11.05.20 15: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(-) Reviewed-by: Max Reitz <mreitz@redhat.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) { >
12.05.2020 20:15, Vladimir Sementsov-Ogievskiy wrote: >> + } 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.. the only one failure case I mean, of course.
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
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>
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; } } + } else if (s->bdev_length != target_length) { + error_setg(errp, "Source and target image have different sizes"); + ret = -EINVAL; + goto immediate_exit; } if (s->bdev_length == 0) {