[v2,3/4] mirror: Make sure that source and target size match
diff mbox series

Message ID 20200511135825.219437-4-kwolf@redhat.com
State New
Headers show
Series
  • mirror: Make sure that source and target size match
Related show

Commit Message

Kevin Wolf May 11, 2020, 1:58 p.m. UTC
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(-)

Comments

Max Reitz May 11, 2020, 3:32 p.m. UTC | #1
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>
Vladimir Sementsov-Ogievskiy May 12, 2020, 5:15 p.m. UTC | #2
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) {
>
Vladimir Sementsov-Ogievskiy May 12, 2020, 5:16 p.m. UTC | #3
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.
Kevin Wolf May 12, 2020, 6:48 p.m. UTC | #4
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
Vladimir Sementsov-Ogievskiy May 13, 2020, 10:44 a.m. UTC | #5
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>

Patch
diff mbox series

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) {