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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@redhat.com>
>> ---
>>   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