Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()

[Max Reitz <mreitz@redhat.com>]

On 10.02.2014 14:30, Benoît Canet wrote:
> Le Saturday 08 Feb 2014 à 18:39:13 (+0100), Max Reitz a écrit :
>> Allow bdrv_open() to handle references to existing block devices just as
>> bdrv_file_open() is already capable of.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 41 ++++++++++++++++++++++++++++++++++-------
>>   block/qcow2.c         |  4 ++--
>>   block/vmdk.c          |  3 ++-
>>   block/vvfat.c         |  2 +-
>>   blockdev.c            | 12 ++++++------
>>   hw/block/xen_disk.c   |  4 ++--
>>   include/block/block.h |  5 +++--
>>   qemu-img.c            |  8 ++++----
>>   qemu-io.c             |  4 +++-
>>   qemu-nbd.c            |  2 +-
>>   10 files changed, 58 insertions(+), 27 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 40a585a..3a32c37 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>       }
>>   
>>       if (!drv->bdrv_file_open) {
>> -        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
>> +        ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
>>           options = NULL;
>>       } else {
>>           ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> @@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>   
>>       assert(bs->backing_hd == NULL);
>>       ret = bdrv_open(&bs->backing_hd,
>> -                    *backing_filename ? backing_filename : NULL, options,
>> +                    *backing_filename ? backing_filename : NULL, NULL, options,
>>                       back_flags, back_drv, &local_err);
>>       if (ret < 0) {
>>           bs->backing_hd = NULL;
>> @@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>               goto done;
>>           }
>>   
>> -        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> +        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>>       } else {
>>           ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>>                                errp);
>> @@ -1221,8 +1221,9 @@ done:
>>    * 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(BlockDriverState **pbs, const char *filename, QDict *options,
>> -              int flags, BlockDriver *drv, Error **errp)
>> +int bdrv_open(BlockDriverState **pbs, const char *filename,
>> +              const char *reference, QDict *options, int flags,
>> +              BlockDriver *drv, Error **errp)
> Maybe a little reference to the reference parameters in the comments would help
> ?

Yes, that would probably be helpful.

Max

> Aside from that:
>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
>
>>   {
>>       int ret;
>>       /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>> @@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>   
>>       assert(pbs);
>>   
>> +    if (reference) {
>> +        bool options_non_empty = options ? qdict_size(options) : false;
>> +        QDECREF(options);
>> +
>> +        if (*pbs) {
>> +            error_setg(errp, "Cannot reuse an existing BDS when referencing "
>> +                       "another block device");
>> +            return -EINVAL;
>> +        }
>> +
>> +        if (filename || options_non_empty) {
>> +            error_setg(errp, "Cannot reference an existing block device with "
>> +                       "additional options or a new filename");
>> +            return -EINVAL;
>> +        }
>> +
>> +        bs = bdrv_find(reference);
>> +        if (!bs) {
>> +            error_setg(errp, "Cannot find block device '%s'", reference);
>> +            return -ENODEV;
>> +        }
>> +        bdrv_ref(bs);
>> +        *pbs = bs;
>> +        return 0;
>> +    }
>> +
>>       if (*pbs) {
>>           bs = *pbs;
>>       } else {
>> @@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>           /* Get the required size from the image */
>>           QINCREF(options);
>>           bs1 = NULL;
>> -        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
>> +        ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
>>                           drv, &local_err);
>>           if (ret < 0) {
>>               goto fail;
>> @@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>                   flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>   
>>               bs = NULL;
>> -            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
>> +            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
>>                               backing_drv, &local_err);
>>               if (ret < 0) {
>>                   error_setg_errno(errp, -ret, "Could not open '%s': %s",
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c8e8ba7..6996276 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>        */
>>       BlockDriver* drv = bdrv_find_format("qcow2");
>>       assert(drv != NULL);
>> -    ret = bdrv_open(&bs, filename, NULL,
>> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>>           BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>> @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>       bs = NULL;
>>   
>>       /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
>> -    ret = bdrv_open(&bs, filename, NULL,
>> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>>                       BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>>                       drv, &local_err);
>>       if (error_is_set(&local_err)) {
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 52cd12a..e007793 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>>       }
>>       if (backing_file) {
>>           BlockDriverState *bs = NULL;
>> -        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>> +        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
>> +                        errp);
>>           if (ret != 0) {
>>               goto exit;
>>           }
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index ae7bc6f..d7a830e 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
>>       }
>>   
>>       s->qcow = NULL;
>> -    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
>> +    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>>               BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>>               &local_err);
>>       if (ret < 0) {
>> diff --git a/blockdev.c b/blockdev.c
>> index 6dd351c..7b7e349 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>       bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>   
>>       QINCREF(bs_opts);
>> -    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>> +    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>>   
>>       if (ret < 0) {
>>           error_setg(errp, "could not open disk image %s: %s",
>> @@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>>       /* TODO Inherit bs->options or only take explicit options with an
>>        * extended QMP command? */
>>       assert(state->new_bs == NULL);
>> -    ret = bdrv_open(&state->new_bs, new_image_file, options,
>> +    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>>                       flags | BDRV_O_NO_BACKING, drv, &local_err);
>>       /* We will manually add the backing_hd field to the bs later */
>>       if (ret != 0) {
>> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>>       Error *local_err = NULL;
>>       int ret;
>>   
>> -    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> @@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
>>       }
>>   
>>       target_bs = NULL;
>> -    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> @@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>>        * file.
>>        */
>>       target_bs = NULL;
>> -    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>> -                    &local_err);
>> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
>> +                    drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 14e6d0b..61e6ff3 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
>>               Error *local_err = NULL;
>>               BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>>                                                              readonly);
>> -            if (bdrv_open(&blkdev->bs,
>> -                          blkdev->filename, NULL, qflags, drv, &local_err) != 0)
>> +            if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
>> +                          drv, &local_err) != 0)
>>               {
>>                   xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>>                                 error_get_pretty(local_err));
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 980869d..a421041 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -190,8 +190,9 @@ 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);
>>   int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> -              int flags, BlockDriver *drv, Error **errp);
>> +int bdrv_open(BlockDriverState **pbs, const char *filename,
>> +              const char *reference, QDict *options, int flags,
>> +              BlockDriver *drv, Error **errp);
>>   BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>                                       BlockDriverState *bs, int flags);
>>   int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 897aa56..b834d52 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>>           drv = NULL;
>>       }
>>   
>> -    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_report("Could not open '%s': %s", filename,
>>                        error_get_pretty(local_err));
>> @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>>   
>>           bs_old_backing = bdrv_new("old_backing");
>>           bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>> -        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>> +        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
>>                           old_backing_drv, &local_err);
>>           if (ret) {
>>               error_report("Could not open old backing file '%s': %s",
>> @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
>>           }
>>           if (out_baseimg[0]) {
>>               bs_new_backing = bdrv_new("new_backing");
>> -            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>> -                        new_backing_drv, &local_err);
>> +            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
>> +                            BDRV_O_FLAGS, new_backing_drv, &local_err);
>>               if (ret) {
>>                   error_report("Could not open new backing file '%s': %s",
>>                                out_baseimg, error_get_pretty(local_err));
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 8da8f6e..61d54c0 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>>       } else {
>>           qemuio_bs = bdrv_new("hda");
>>   
>> -        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>> +        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
>> +            < 0)
>> +        {
>>               fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>>                       error_get_pretty(local_err));
>>               error_free(local_err);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 0cf123c..711162c 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>>   
>>       bs = bdrv_new("hda");
>>       srcpath = argv[optind];
>> -    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           errno = -ret;
>>           err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
>> -- 
>> 1.8.5.4
>>
>>