Re: [Qemu-devel] [PATCH v13 05/14] block: Add bdrv_set_backing_hd()

[Fam Zheng]

On Thu, 02/13 13:49, Benoît Canet wrote:
> The Wednesday 29 Jan 2014 à 13:07:32 (+0800), Fam Zheng wrote :
> > This is the common but non-trivial steps to assign or change the
> > backing_hd of BDS.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  block.c               | 34 +++++++++++++++++++++++++---------
> >  include/block/block.h |  1 +
> >  2 files changed, 26 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 8000ac0..c4eaa37 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1072,6 +1072,26 @@ fail:
> >      return ret;
> >  }
> >  
> > +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState 
> > *backing_hd)
> > +{
> > +    if (bs->backing_hd) {
> > +        bdrv_unref(bs->backing_hd);
> > +    }
> > +
> > +    bs->backing_hd = backing_hd;
> > +    if (!backing_hd) {
> > +        bs->backing_file[0] = '\0';
> > +        bs->backing_format[0] = '\0';
> > +        goto out;
> > +    }
> > +    pstrcpy(bs->backing_file, sizeof(bs->backing_file), 
> > backing_hd->filename);
> 
> 
> > +    pstrcpy(bs->backing_format, sizeof(bs->backing_file),
> 
> I don't understand the sizeof(bs->backing_file). Is it 
> sizeof(bs->backing_format) ?
> 

Yes.

> > +            backing_hd->drv ? backing_hd->drv->format_name : "");
> > +    bdrv_ref(bs->backing_hd);
> > +out:
> > +    bdrv_refresh_limits(bs);
> > +}
> > +
> >  /*
> >   * Opens the backing file for a BlockDriverState if not yet open
> >   *
> > @@ -1085,6 +1105,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, 
> > QDict *options, Error **errp)
> >      char backing_filename[PATH_MAX];
> >      int back_flags, ret;
> >      BlockDriver *back_drv = NULL;
> > +    BlockDriverState *backing_hd;
> >      Error *local_err = NULL;
> >  
> >      if (bs->backing_hd != NULL) {
> > @@ -1108,7 +1129,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, 
> > QDict *options, Error **errp)
> >                                         sizeof(backing_filename));
> >      }
> >  
> > -    bs->backing_hd = bdrv_new("");
> > +    backing_hd = bdrv_new("");
> >  
> >      if (bs->backing_format[0] != '\0') {
> >          back_drv = bdrv_find_format(bs->backing_format);
> > @@ -1118,23 +1139,18 @@ int bdrv_open_backing_file(BlockDriverState *bs, 
> > QDict *options, Error **errp)
> >      back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> >                                      BDRV_O_COPY_ON_READ);
> >  
> > -    ret = bdrv_open(bs->backing_hd,
> > +    ret = bdrv_open(backing_hd,
> >                      *backing_filename ? backing_filename : NULL, options,
> >                      back_flags, back_drv, &local_err);
> >      if (ret < 0) {
> > -        bdrv_unref(bs->backing_hd);
> > -        bs->backing_hd = NULL;
> > +        bdrv_unref(backing_hd);
> Here I wonder if this way of calling bdrv_open and doing a bdrv_unref confict
> with Max latests bdrv_open patches.
> You probably need to rebase and get rid of this bdrv_unref() : check Max 
> series
> to be sure.

I'll check it.

> 
> >          bs->open_flags |= BDRV_O_NO_BACKING;
> >          error_setg(errp, "Could not open backing file: %s",
> >                     error_get_pretty(local_err));
> >          error_free(local_err);
> >          return ret;
> >      }
> > -
> > -    if (bs->backing_hd->file) {
> > -        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> > -                bs->backing_hd->file->filename);
> > -    }
> > +    bdrv_set_backing_hd(bs, backing_hd);
> >  
> >      /* Recalculate the BlockLimits with the backing file */
> >      bdrv_refresh_limits(bs);
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 9125bbe..f449753 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -208,6 +208,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char 
> > *filename,
> >  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> >                      QDict *options, const char *bdref_key, int flags,
> >                      bool force_raw, bool allow_none, Error **errp);
> > +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState 
> > *backing_hd);
> >  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error 
> > **errp);
> >  int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> >                int flags, BlockDriver *drv, Error **errp);
> > -- 
> > 1.8.5.3
> > 
> > 
> 
> bdrv_set_backing_hd seems to be handy since some people want to dynamically
> change backing_file using QMP to move them from slow storage to fast storage 
> and
> the reverse.
> 

Thanks,
Fam