From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45289) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDNTV-0008Mm-Ol for qemu-devel@nongnu.org; Tue, 11 Feb 2014 19:14:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WDNTO-0008Iy-4G for qemu-devel@nongnu.org; Tue, 11 Feb 2014 19:14:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:4910) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDNTN-0008IX-Qa for qemu-devel@nongnu.org; Tue, 11 Feb 2014 19:14:42 -0500 Message-ID: <52FABD80.3010008@redhat.com> Date: Wed, 12 Feb 2014 01:17:04 +0100 From: Max Reitz MIME-Version: 1.0 References: <1391881159-13585-1-git-send-email-mreitz@redhat.com> <1391881159-13585-3-git-send-email-mreitz@redhat.com> <20140210133016.GG3088@irqsave.net> In-Reply-To: <20140210133016.GG3088@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Stefan Hajnoczi On 10.02.2014 14:30, Beno=EEt Canet wrote: > Le Saturday 08 Feb 2014 =E0 18:39:13 (+0100), Max Reitz a =E9crit : >> Allow bdrv_open() to handle references to existing block devices just = as >> bdrv_file_open() is already capable of. >> >> Signed-off-by: Max Reitz >> --- >> 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, >> } >> =20 >> if (!drv->bdrv_file_open) { >> - ret =3D bdrv_open(&bs, filename, options, flags, drv, &local_= err); >> + ret =3D bdrv_open(&bs, filename, NULL, options, flags, drv, &= local_err); >> options =3D NULL; >> } else { >> ret =3D bdrv_open_common(bs, NULL, options, flags, drv, &loc= al_err); >> @@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs,= QDict *options, Error **errp) >> =20 >> assert(bs->backing_hd =3D=3D NULL); >> ret =3D bdrv_open(&bs->backing_hd, >> - *backing_filename ? backing_filename : NULL, opti= ons, >> + *backing_filename ? backing_filename : NULL, NULL= , options, >> back_flags, back_drv, &local_err); >> if (ret < 0) { >> bs->backing_hd =3D NULL; >> @@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, cons= t char *filename, >> goto done; >> } >> =20 >> - ret =3D bdrv_open(pbs, filename, image_options, flags, NULL, = errp); >> + ret =3D bdrv_open(pbs, filename, NULL, image_options, flags, = NULL, errp); >> } else { >> ret =3D bdrv_file_open(pbs, filename, reference, image_optio= ns, flags, >> errp); >> @@ -1221,8 +1221,9 @@ done: >> * If *pbs is NULL, a new BDS will be created with a pointer to it s= tored there. >> * If it is not NULL, the referenced BDS will be reused. >> */ >> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *op= tions, >> - 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 wo= uld help > ? Yes, that would probably be helpful. Max > Aside from that: > > Reviewed-by: Benoit Canet > >> { >> int ret; >> /* TODO: extra byte is a hack to ensure MAX_PATH space on Window= s. */ >> @@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const cha= r *filename, QDict *options, >> =20 >> assert(pbs); >> =20 >> + if (reference) { >> + bool options_non_empty =3D options ? qdict_size(options) : fa= lse; >> + QDECREF(options); >> + >> + if (*pbs) { >> + error_setg(errp, "Cannot reuse an existing BDS when refer= encing " >> + "another block device"); >> + return -EINVAL; >> + } >> + >> + if (filename || options_non_empty) { >> + error_setg(errp, "Cannot reference an existing block devi= ce with " >> + "additional options or a new filename"); >> + return -EINVAL; >> + } >> + >> + bs =3D bdrv_find(reference); >> + if (!bs) { >> + error_setg(errp, "Cannot find block device '%s'", referen= ce); >> + return -ENODEV; >> + } >> + bdrv_ref(bs); >> + *pbs =3D bs; >> + return 0; >> + } >> + >> if (*pbs) { >> bs =3D *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 =3D NULL; >> - ret =3D bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING, >> + ret =3D bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BA= CKING, >> 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); >> =20 >> bs =3D NULL; >> - ret =3D bdrv_open(&bs, backing_file->value.s, NULL, back_= flags, >> + ret =3D 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, i= nt64_t total_size, >> */ >> BlockDriver* drv =3D bdrv_find_format("qcow2"); >> assert(drv !=3D NULL); >> - ret =3D bdrv_open(&bs, filename, NULL, >> + ret =3D 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, i= nt64_t total_size, >> bs =3D NULL; >> =20 >> /* Reopen the image without BDRV_O_NO_FLUSH to flush it before r= eturning */ >> - ret =3D bdrv_open(&bs, filename, NULL, >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, >> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKIN= G, >> 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, QEM= UOptionParameter *options, >> } >> if (backing_file) { >> BlockDriverState *bs =3D NULL; >> - ret =3D bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING,= NULL, errp); >> + ret =3D bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BA= CKING, NULL, >> + errp); >> if (ret !=3D 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= ) >> } >> =20 >> s->qcow =3D NULL; >> - ret =3D bdrv_open(&s->qcow, s->qcow_filename, NULL, >> + ret =3D bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL, >> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qc= ow, >> &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 |=3D ro ? 0 : BDRV_O_RDWR; >> =20 >> QINCREF(bs_opts); >> - ret =3D bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &= error); >> + ret =3D bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, = drv, &error); >> =20 >> if (ret < 0) { >> error_setg(errp, "could not open disk image %s: %s", >> @@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransac= tionState *common, >> /* TODO Inherit bs->options or only take explicit options with a= n >> * extended QMP command? */ >> assert(state->new_bs =3D=3D NULL); >> - ret =3D bdrv_open(&state->new_bs, new_image_file, options, >> + ret =3D 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 !=3D 0) { >> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverS= tate *bs, const char *filename, >> Error *local_err =3D NULL; >> int ret; >> =20 >> - ret =3D bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_er= r); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &lo= cal_err); >> if (ret < 0) { >> error_propagate(errp, local_err); >> return; >> @@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const = char *target, >> } >> =20 >> target_bs =3D NULL; >> - ret =3D bdrv_open(&target_bs, target, NULL, flags, drv, &local_er= r); >> + ret =3D bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &lo= cal_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 =3D NULL; >> - ret =3D bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BAC= KING, drv, >> - &local_err); >> + ret =3D 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 =3D NULL; >> BlockDriver *drv =3D bdrv_find_whitelisted_format(blkdev= ->fileproto, >> readonly)= ; >> - if (bdrv_open(&blkdev->bs, >> - blkdev->filename, NULL, qflags, drv, &local= _err) !=3D 0) >> + if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, = qflags, >> + drv, &local_err) !=3D 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, Err= or **errp); >> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *op= tions, >> - 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 =3D NULL; >> } >> =20 >> - ret =3D bdrv_open(&bs, filename, NULL, flags, drv, &local_err); >> + ret =3D bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_e= rr); >> 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) >> =20 >> bs_old_backing =3D bdrv_new("old_backing"); >> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_n= ame)); >> - ret =3D bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O= _FLAGS, >> + ret =3D 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 =3D bdrv_new("new_backing"); >> - ret =3D bdrv_open(&bs_new_backing, out_baseimg, NULL, BDR= V_O_FLAGS, >> - new_backing_drv, &local_err); >> + ret =3D bdrv_open(&bs_new_backing, out_baseimg, NULL, NUL= L, >> + 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 growa= ble, QDict *opts) >> } else { >> qemuio_bs =3D bdrv_new("hda"); >> =20 >> - if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err= ) < 0) { >> + if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &loc= al_err) >> + < 0) >> + { >> fprintf(stderr, "%s: can't open device %s: %s\n", progna= me, 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) >> =20 >> bs =3D bdrv_new("hda"); >> srcpath =3D argv[optind]; >> - ret =3D bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err); >> + ret =3D bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_er= r); >> if (ret < 0) { >> errno =3D -ret; >> err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optin= d], >> --=20 >> 1.8.5.4 >> >>