Re: [Qemu-devel] [PATCH 03/10] block: Make bdrv_file_open() static

[Max Reitz <mreitz@redhat.com>]

On 29.01.2014 14:26, Kevin Wolf wrote:
> Am 26.01.2014 um 20:02 hat Max Reitz geschrieben:
>> Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
>> call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
>> therefore bdrv_open() the only way to call it.
>>
>> Consequently, all existing calls to bdrv_file_open() have to be adjusted
>> to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 17 ++++++++++++-----
>>   block/cow.c           |  6 +++---
>>   block/qcow.c          |  6 +++---
>>   block/qcow2.c         |  5 +++--
>>   block/qed.c           |  5 +++--
>>   block/sheepdog.c      |  8 +++++---
>>   block/vhdx.c          |  5 +++--
>>   block/vmdk.c          | 11 +++++++----
>>   include/block/block.h |  5 ++---
>>   qemu-io.c             |  4 +++-
>>   10 files changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f9923bb..0fb7892 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -947,9 +947,9 @@ free_and_fail:
>>    * after the call (even on failure), so if the caller intends to reuse the
>>    * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
>>    */
>> -int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> -                   const char *reference, QDict *options, int flags,
>> -                   Error **errp)
>> +static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>> +                          const char *reference, QDict *options, int flags,
>> +                          Error **errp)
>>   {
>>       BlockDriverState *bs = NULL;
>>       BlockDriver *drv;
>> @@ -1196,8 +1196,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>           *pbs = NULL;
>>           ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>>       } else {
>> -        ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>> -                             errp);
>> +        *pbs = NULL;
>> +        ret = bdrv_open(pbs, filename, reference, image_options,
>> +                        flags | BDRV_O_PROTOCOL, NULL, errp);
>>       }
>>   
>>   done:
>> @@ -1227,6 +1228,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>>       const char *drvname;
>>       Error *local_err = NULL;
>>   
>> +    if (flags & BDRV_O_PROTOCOL) {
>> +        assert(!drv);
>> +        return bdrv_file_open(pbs, filename, reference, options,
>> +                              flags & ~BDRV_O_PROTOCOL, errp);
>> +    }
>> +
>>       /* NULL means an empty set of options */
>>       if (options == NULL) {
>>           options = qdict_new();
>> diff --git a/block/cow.c b/block/cow.c
>> index 7fc0b12..f0748a2 100644
>> --- a/block/cow.c
>> +++ b/block/cow.c
>> @@ -332,7 +332,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>>       const char *image_filename = NULL;
>>       Error *local_err = NULL;
>>       int ret;
>> -    BlockDriverState *cow_bs;
>> +    BlockDriverState *cow_bs = NULL;
>>   
>>       /* Read out options */
>>       while (options && options->name) {
>> @@ -351,8 +351,8 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>>           return ret;
>>       }
>>   
>> -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
>> -                         &local_err);
>> +    ret = bdrv_open(&cow_bs, filename, NULL, NULL,
>> +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
>>       if (ret < 0) {
>>           qerror_report_err(local_err);
>>           error_free(local_err);
>> diff --git a/block/qcow.c b/block/qcow.c
>> index 948b0c5..4881d84 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -670,7 +670,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
>>       int flags = 0;
>>       Error *local_err = NULL;
>>       int ret;
>> -    BlockDriverState *qcow_bs;
>> +    BlockDriverState *qcow_bs = NULL;
>>   
>>       /* Read out options */
>>       while (options && options->name) {
>> @@ -691,8 +691,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
>>           return ret;
>>       }
>>   
>> -    ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
>> -                         &local_err);
>> +    ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
>> +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
>>       if (ret < 0) {
>>           qerror_report_err(local_err);
>>           error_free(local_err);
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 7202a26..40b957b 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1479,7 +1479,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>        * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
>>        * size for any qcow2 image.
>>        */
>> -    BlockDriverState* bs;
>> +    BlockDriverState *bs = NULL;
>>       QCowHeader *header;
>>       uint8_t* refcount_table;
>>       Error *local_err = NULL;
>> @@ -1491,7 +1491,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>           return ret;
>>       }
>>   
>> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>> +                    NULL, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return ret;
>> diff --git a/block/qed.c b/block/qed.c
>> index 694e6e2..dee2e61 100644
>> --- a/block/qed.c
>> +++ b/block/qed.c
>> @@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32_t cluster_size,
>>           return ret;
>>       }
>>   
>> -    ret = bdrv_file_open(&bs, filename, NULL, NULL,
>> -                         BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>> +                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL,
>> +                    &local_err);
>>       if (ret < 0) {
>>           qerror_report_err(local_err);
>>           error_free(local_err);
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 672b9c9..6f2ec57 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename)
>>       Error *local_err = NULL;
>>       int ret;
>>   
>> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>> +                    NULL, &local_err);
>>       if (ret < 0) {
>>           qerror_report_err(local_err);
>>           error_free(local_err);
>> @@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
>>       }
>>   
>>       if (backing_file) {
>> -        BlockDriverState *bs;
>> +        BlockDriverState *bs = NULL;
>>           BDRVSheepdogState *base;
>>           BlockDriver *drv;
>>   
>> @@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
>>               goto out;
>>           }
>>   
>> -        ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err);
>> +        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
>> +                        &local_err);
>>           if (ret < 0) {
>>               qerror_report_err(local_err);
>>               error_free(local_err);
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index 9ee0a61..13513b4 100644
>> --- a/block/vhdx.c
>> +++ b/block/vhdx.c
>> @@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>>   
>>       gunichar2 *creator = NULL;
>>       glong creator_items;
>> -    BlockDriverState *bs;
>> +    BlockDriverState *bs = NULL;
>>       const char *type = NULL;
>>       VHDXImageType image_type;
>>       Error *local_err = NULL;
>> @@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>>           goto exit;
>>       }
>>   
>> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>> +                    NULL, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           goto exit;
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 37b2bc8..d2a69d5 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>   
>>           path_combine(extent_path, sizeof(extent_path),
>>                   desc_file_path, fname);
>> -        ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
>> -                             bs->open_flags, errp);
>> +        extent_file = NULL;
>> +        ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
>> +                        bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
>>           if (ret) {
>>               return ret;
>>           }
>> @@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>>           goto exit;
>>       }
>>   
>> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>> +                    NULL, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           goto exit;
>> @@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>>               goto exit;
>>           }
>>       }
>> -    ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
>> +    ret = bdrv_open(&new_bs, filename, NULL, NULL,
>> +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
>>       if (ret < 0) {
>>           error_setg_errno(errp, -ret, "Could not write description");
>>           goto exit;
>> diff --git a/include/block/block.h b/include/block/block.h
>> index a421041..396f9ed 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -102,6 +102,8 @@ typedef enum {
>>   #define BDRV_O_CHECK       0x1000  /* open solely for consistency check */
>>   #define BDRV_O_ALLOW_RDWR  0x2000  /* allow reopen to change from r/o to r/w */
>>   #define BDRV_O_UNMAP       0x4000  /* execute guest UNMAP/TRIM operations */
>> +#define BDRV_O_PROTOCOL    0x8000  /* open the file using a protocol instead of
>> +                                      a block driver */
> Protocol drivers are a subset of block drivers, so this description
> doesn't make sense.

Hm, technically they probably are but it always seemed to me that 
bdrv_open() would never directly use a protocol, instead using raw as 
the format if no format was found.

More importantly, it is actually possible to use a non-protocol block 
driver with BDRV_O_PROTOCOL; it just needs to be explicitly specified.

> I guess we need to list the differences between bdrv_open() and
> bdrv_file_open() in order to define what this flag really changes. I
> think this includes:
>
> - Disables format probing
> - BDRV_O_SNAPSHOT is ignored
> - No backing files are opened
> - Probably a few more

So, to me, the main difference is that bdrv_open() always uses some 
non-protocol block driver, whereas bdrv_file_open() only probes for 
protocol block drivers (if a non-protocol driver should be used, it has 
to be explicitly specified).

The current comment for bdrv_file_open() doesn't help much, either: 
“Opens a file using a protocol”. Therefore, the easiest way would just 
be to state “behaves like the old bdrv_file_open()”, but that would not 
be very helpful. Perhaps we could formulate it like “Opens a single file 
(no backing chain, etc.) using only a protocol driver deduced from the 
filename, if not explicitly specified otherwise.”

Max