From: Max Reitz <mreitz@redhat.com>
To: "Benoît Canet" <benoit.canet@irqsave.net>
Cc: Kevin Wolf <kwolf@redhat.com>, Jeff Cody <jcody@redhat.com>,
qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()
Date: Wed, 12 Feb 2014 01:17:04 +0100 [thread overview]
Message-ID: <52FABD80.3010008@redhat.com> (raw)
In-Reply-To: <20140210133016.GG3088@irqsave.net>
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
>>
>>
next prev parent reply other threads:[~2014-02-12 0:14 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-08 17:39 [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** Max Reitz
2014-02-10 12:42 ` Kevin Wolf
2014-02-12 0:10 ` Max Reitz
2014-02-10 13:17 ` Benoît Canet
2014-02-12 0:15 ` Max Reitz
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open() Max Reitz
2014-02-10 13:30 ` Benoît Canet
2014-02-12 0:17 ` Max Reitz [this message]
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static Max Reitz
2014-02-10 13:40 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open() Max Reitz
2014-02-10 13:42 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open() Max Reitz
2014-02-10 13:49 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only Max Reitz
2014-02-10 16:23 ` Benoît Canet
2014-02-10 16:23 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open() Max Reitz
2014-02-10 14:56 ` Kevin Wolf
2014-02-12 0:26 ` Max Reitz
2014-02-10 16:28 ` Benoît Canet
2014-02-08 17:39 ` [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option Max Reitz
2014-02-10 16:31 ` Benoît Canet
2014-02-10 15:01 ` [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open() Kevin Wolf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52FABD80.3010008@redhat.com \
--to=mreitz@redhat.com \
--cc=benoit.canet@irqsave.net \
--cc=jcody@redhat.com \
--cc=kwolf@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.