Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file

[Paolo Bonzini <pbonzini@redhat.com>]

> Once again, combining code motion and code changes in one patch makes
> it harder to review.

bdrv_ensure_backing_file() is a new standalone function that happens to be
usable in bdrv_open as well.  But I can separate the changes/fixes to a
separate patch.

In particular it is can be used after a file has been opened with
BDRV_O_NO_BACKING (at which point the flag does not reflect reality
anymore, hence the removal of the flag).

> bdrv_ensure_backing_file() isn't a good name, after reading only the
> subject line I had no idea what this function might do. It's still
> not entirely clear to me what the different to a bdrv_open_backing_file()
> is, except that it doesn't do anything if a backing file is already
> open. In which cases do we rely on this behaviour?

We open the mirroring target with BDRV_O_NO_BACKING usually, but require
the backing file if the cluster size is larger than the dirty block
granularity.  Later, COW is done in the mirror job, so this is not
needed anymore at the end of the series.
 
> > ---
> >  block.c |   69
> >  ++++++++++++++++++++++++++++++++++++++++-----------------------
> >  block.h |    1 +
> >  2 files changed, 45 insertions(+), 25 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 19da114..002b442 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -730,6 +730,48 @@ int bdrv_file_open(BlockDriverState **pbs,
> > const char *filename, int flags)
> >      return 0;
> >  }
> >  
> > +int bdrv_ensure_backing_file(BlockDriverState *bs)
> > +{
> > +    char backing_filename[PATH_MAX];
> > +    int back_flags, ret;
> > +    BlockDriver *back_drv = NULL;
> > +
> > +    if (bs->backing_hd != NULL) {
> > +        return 0;
> > +    }
> > +
> > +    bs->open_flags &= ~BDRV_O_NO_BACKING;
> 
> This doesn't do anything in this patch because the function is never
> called with BDRV_O_NO_BACKING set. Is it in preparation for a second
> user?

Yes, see above.

> > +    if (bs->backing_file[0] == '\0') {
> > +        return 0;
> > +    }
> > +
> > +    bs->backing_hd = bdrv_new("");
> > +    bdrv_get_full_backing_filename(bs, backing_filename,
> > +                                   sizeof(backing_filename));
> > +
> > +    if (bs->backing_format[0] != '\0') {
> > +        back_drv = bdrv_find_format(bs->backing_format);
> > +    }
> > +
> > +    /* backing files always opened read-only */
> > +    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
> > +
> > +    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags,
> > back_drv);
> > +    if (ret < 0) {
> > +        bdrv_close(bs);
> 
> I don't like this because it makes the invalid assumption that the
> caller has just opened bs and wants to close it if opening the
> backing file fails. I think this is part of the error handling that belongs
> in the caller: It opened bs, so it is responsible for closing it in
> error cases.

It's a bug, it should have closed bs->backing_hd.

> > +        bdrv_delete(bs->backing_hd);
> 
> This is a bug fix of its own, should be a separate patch.

Ok.

> > +        bs->backing_hd = NULL;
> > +        return ret;
> > +    }
> > +    if (bs->is_temporary) {
> > +        bs->backing_hd->keep_read_only = !(bs->open_flags &
> > BDRV_O_RDWR);
> > +    } else {
> > +        /* base images use the same setting as leaf */
> 
> Huh, "parent" turned into "leaf"?

Will move this to a separate patch, too.

Paolo

> > +        bs->backing_hd->keep_read_only = bs->keep_read_only;
> > +    }
> > +    return 0;
> > +}
> 
> Kevin
>