Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **

[Max Reitz]

On 10.02.2014 13:42, Kevin Wolf wrote:
> Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> > Make bdrv_open() take a pointer to a BDS pointer, similarly to
> > bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
> > will create a new BDS with an empty name; if the BDS pointer is not
> > NULL, that existing BDS will be reused (in the same way as bdrv_open()
> > already did).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block.c               | 64 +++++++++++++++++++++++++++++++--------------------
> >   block/blkdebug.c      |  1 +
> >   block/blkverify.c     |  2 ++
> >   block/qcow2.c         | 14 +++++++----
> >   block/vmdk.c          |  5 ++--
> >   block/vvfat.c         |  6 ++---
> >   blockdev.c            | 20 ++++++++--------
> >   hw/block/xen_disk.c   |  2 +-
> >   include/block/block.h |  2 +-
> >   qemu-img.c            | 10 ++++----
> >   qemu-io.c             |  2 +-
> >   qemu-nbd.c            |  2 +-
> >   12 files changed, 72 insertions(+), 58 deletions(-)
> > @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
> > *options, Error **errp)
> >    * BlockdevRef.
> >    *
> >    * The BlockdevRef will be removed from the options QDict.
> > + *
> > + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
> > + * pointer to it stored there. If it is not NULL, the referenced BDS will
> > + * be reused.
> >    */
> >   int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> >                       QDict *options, const char *bdref_key, int flags,
> There are no callers that make use of *pbs != NULL. Are you planning to
> add such users? Otherwise, we could just assert() it here instead of
> documenting behaviour that is never used.

No, currently, there aren't. Since we're planning to eventually adjust 
everything to give a pointer to a NULL pointer to these functions (i.e., 
nobody except bdrv_open() uses bdrv_new()), adding an assert() in order 
to prevent anyone from "exploiting" this in a way which would 
technically be fine but actually not what we want (i.e., reusing an 
already existing BDS), is fine with me. I'll add it and adjust the comment.

Max