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