mirror: Check for bdrv_get_info result

bdrv_get_info could fail. Add check before using the returned value.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Fri, Apr 25, 2014 at 02:50:29PM +0800, Fam Zheng wrote:
> bdrv_get_info could fail. Add check before using the returned value.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 0ef41f9..fafcc93 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -339,8 +339,8 @@ static void coroutine_fn mirror_run(void *opaque)
>      bdrv_get_backing_filename(s->target, backing_filename,
>                                sizeof(backing_filename));
>      if (backing_filename[0] && !s->target->backing_hd) {
> -        bdrv_get_info(s->target, &bdi);
> -        if (s->granularity < bdi.cluster_size) {
> +        ret = bdrv_get_info(s->target, &bdi);
> +        if (ret == 0 && s->granularity < bdi.cluster_size) {
>              s->buf_size = MAX(s->buf_size, bdi.cluster_size);
>              s->cow_bitmap = bitmap_new(length);
>          }

Please explain why it's okay to ignore errors.

Stefan

On Fri, 04/25 14:20, Stefan Hajnoczi wrote:
> On Fri, Apr 25, 2014 at 02:50:29PM +0800, Fam Zheng wrote:
> > bdrv_get_info could fail. Add check before using the returned value.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/mirror.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 0ef41f9..fafcc93 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -339,8 +339,8 @@ static void coroutine_fn mirror_run(void *opaque)
> >      bdrv_get_backing_filename(s->target, backing_filename,
> >                                sizeof(backing_filename));
> >      if (backing_filename[0] && !s->target->backing_hd) {
> > -        bdrv_get_info(s->target, &bdi);
> > -        if (s->granularity < bdi.cluster_size) {
> > +        ret = bdrv_get_info(s->target, &bdi);
> > +        if (ret == 0 && s->granularity < bdi.cluster_size) {
> >              s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> >              s->cow_bitmap = bitmap_new(length);
> >          }
> 
> Please explain why it's okay to ignore errors.
> 

No it's not OK. Thank you for noticing.

Previously, when target cluster size is larger than s->granularity, COW is
enabled and data must be copied by target cluster.

Otherwise, if we copy less, which covers only a partial cluster in target, it
will allocate a full cluster, without copying original data from source, this
part of the cluster is still zero, and may be left over that way, if the
corresponding data from source is never copied according to sync mode (top or
none, for example).

With this change, if we get an error in bdrv_get_info() here, we don't know the
target cluster size, and will skip this COW in mirror job. That is risky for
above reason.

To be safe, we should error out. Will respin.

Thanks,
Fam