From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44794) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDNS7-0007Cl-Td for qemu-devel@nongnu.org; Tue, 11 Feb 2014 19:13:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WDNS0-0007ym-Q4 for qemu-devel@nongnu.org; Tue, 11 Feb 2014 19:13:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:21369) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDNS0-0007yi-FY for qemu-devel@nongnu.org; Tue, 11 Feb 2014 19:13:16 -0500 Message-ID: <52FABD2A.9060601@redhat.com> Date: Wed, 12 Feb 2014 01:15:38 +0100 From: Max Reitz MIME-Version: 1.0 References: <1391881159-13585-1-git-send-email-mreitz@redhat.com> <1391881159-13585-2-git-send-email-mreitz@redhat.com> <20140210131737.GE3088@irqsave.net> In-Reply-To: <20140210131737.GE3088@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to ** 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:17, Beno=EEt Canet wrote: > Le Saturday 08 Feb 2014 =E0 18:39:12 (+0100), Max Reitz a =E9crit : >> 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 >> --- >> 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(-) >> >> diff --git a/block.c b/block.c >> index 636aa11..40a585a 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_e= rr); >> + ret =3D bdrv_open(&bs, filename, options, flags, drv, &local_= err); >> options =3D NULL; >> } else { >> ret =3D bdrv_open_common(bs, NULL, options, flags, drv, &loc= al_err); >> @@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs,= QDict *options, Error **errp) >> sizeof(backing_filename)); >> } >> =20 >> - bs->backing_hd =3D bdrv_new(""); >> - >> if (bs->backing_format[0] !=3D '\0') { >> back_drv =3D bdrv_find_format(bs->backing_format); >> } >> @@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *b= s, QDict *options, Error **errp) >> back_flags =3D bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT = | >> BDRV_O_COPY_ON_READ); >> =20 >> - ret =3D bdrv_open(bs->backing_hd, >> + assert(bs->backing_hd =3D=3D NULL); >> + ret =3D bdrv_open(&bs->backing_hd, >> *backing_filename ? backing_filename : NULL, opt= ions, >> back_flags, back_drv, &local_err); >> if (ret < 0) { >> - bdrv_unref(bs->backing_hd); >> bs->backing_hd =3D NULL; >> bs->open_flags |=3D BDRV_O_NO_BACKING; >> error_setg(errp, "Could not open backing file: %s", >> @@ -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 wi= th 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= , >> @@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, cons= t char *filename, >> /* If a filename is given and the block driver should be det= ected >> automatically (instead of using none), use bdrv_open() in= order to do >> that auto-detection. */ >> - BlockDriverState *bs; >> - >> if (reference) { >> error_setg(errp, "Cannot reference an existing block dev= ice while " >> "giving a filename"); >> @@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, con= st char *filename, >> goto done; >> } >> =20 >> - bs =3D bdrv_new(""); >> - ret =3D bdrv_open(bs, filename, image_options, flags, NULL, e= rrp); >> - if (ret < 0) { >> - bdrv_unref(bs); >> - } else { >> - *pbs =3D bs; >> - } >> + ret =3D bdrv_open(pbs, filename, image_options, flags, NULL, = errp); >> } else { >> ret =3D bdrv_file_open(pbs, filename, reference, image_optio= ns, flags, >> errp); >> @@ -1223,22 +1217,32 @@ done: >> * empty set of options. The reference to the QDict belongs to the b= lock layer >> * after the call (even on failure), so if the caller intends to reu= se the >> * dictionary, it needs to use QINCREF() before calling bdrv_open. >> + * >> + * If *pbs is NULL, a new BDS will be created with a pointer to it st= ored there. >> + * If it is not NULL, the referenced BDS will be reused. >> */ >> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *opti= ons, >> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *op= tions, >> int flags, BlockDriver *drv, Error **errp) >> { >> int ret; >> /* TODO: extra byte is a hack to ensure MAX_PATH space on Window= s. */ >> char tmp_filename[PATH_MAX + 1]; >> - BlockDriverState *file =3D NULL; >> + BlockDriverState *file =3D NULL, *bs; >> const char *drvname; >> Error *local_err =3D NULL; >> =20 >> + assert(pbs); >> + >> + if (*pbs) { >> + bs =3D *pbs; >> + } else { >> + bs =3D bdrv_new(""); >> + } >> + >> /* NULL means an empty set of options */ >> if (options =3D=3D NULL) { >> options =3D qdict_new(); >> } >> - > nitpicking: spurious line removal. Right, I didn't notice because I mostly compared v2 against v1. :-) >> bs->options =3D options; >> options =3D qdict_clone_shallow(options); >> =20 >> @@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char= *filename, QDict *options, >> instead of opening 'filename' directly */ >> =20 >> /* Get the required size from the image */ >> - bs1 =3D bdrv_new(""); >> QINCREF(options); >> - ret =3D bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING, >> + bs1 =3D NULL; >> + ret =3D bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING, >> drv, &local_err); >> if (ret < 0) { >> - bdrv_unref(bs1); >> goto fail; >> } >> total_size =3D bdrv_getlength(bs1) & BDRV_SECTOR_MASK; >> @@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *= filename, QDict *options, >> flags |=3D BDRV_O_ALLOW_RDWR; >> } >> =20 >> + assert(file =3D=3D NULL); >> ret =3D bdrv_open_image(&file, filename, options, "file", >> bdrv_open_flags(bs, flags | BDRV_O_UNMAP),= true, true, >> &local_err); > is_temporary can already be set to one the goto after the bdrv_open_ima= ge should > be unlink_and_fail yet I don't know if the fix belong to this patch. Oh, right, good catch. I don't think this belongs to this series,=20 though. Do you want to fix this or shall I? Max >> @@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *= filename, QDict *options, >> bdrv_dev_change_media_cb(bs, true); >> } >> =20 >> + *pbs =3D bs; >> return 0; >> =20 >> unlink_and_fail: >> @@ -1400,13 +1405,24 @@ fail: >> QDECREF(bs->options); >> QDECREF(options); >> bs->options =3D NULL; >> + if (!*pbs) { >> + /* If *pbs is NULL, a new BDS has been created in this functi= on and >> + needs to be freed now. Otherwise, it does not need to be c= losed, >> + since it has not really been opened yet. */ >> + bdrv_unref(bs); >> + } >> if (error_is_set(&local_err)) { >> error_propagate(errp, local_err); >> } >> return ret; >> =20 >> close_and_fail: >> - bdrv_close(bs); >> + /* See fail path, but now the BDS has to be always closed */ >> + if (*pbs) { >> + bdrv_close(bs); >> + } else { >> + bdrv_unref(bs); >> + } >> QDECREF(options); >> if (error_is_set(&local_err)) { >> error_propagate(errp, local_err); >> @@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const= char *fmt, >> back_flags =3D >> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_= BACKING); >> =20 >> - bs =3D bdrv_new(""); >> - >> - ret =3D bdrv_open(bs, backing_file->value.s, NULL, back_f= lags, >> + bs =3D NULL; >> + ret =3D bdrv_open(&bs, backing_file->value.s, NULL, back_= flags, >> backing_drv, &local_err); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Could not open '%s': %= s", >> @@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const= char *fmt, >> error_get_pretty(local_err)); >> error_free(local_err); >> local_err =3D NULL; >> - bdrv_unref(bs); >> goto out; >> } >> bdrv_get_geometry(bs, &size); >> diff --git a/block/blkdebug.c b/block/blkdebug.c >> index 8eb0db0..053fa4c 100644 >> --- a/block/blkdebug.c >> +++ b/block/blkdebug.c >> @@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDi= ct *options, int flags, >> s->state =3D 1; >> =20 >> /* Open the backing file */ >> + assert(bs->file =3D=3D NULL); >> ret =3D bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image")= , options, "image", >> flags, true, false, &local_err); >> if (ret < 0) { >> diff --git a/block/blkverify.c b/block/blkverify.c >> index cfcbcf4..86585e7 100644 >> --- a/block/blkverify.c >> +++ b/block/blkverify.c >> @@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QD= ict *options, int flags, >> } >> =20 >> /* Open the raw file */ >> + assert(bs->file =3D=3D NULL); >> ret =3D bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), = options, >> "raw", flags, true, false, &local_err); >> if (ret < 0) { >> @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QD= ict *options, int flags, >> } >> =20 >> /* Open the test file */ >> + assert(s->test_file =3D=3D NULL); >> ret =3D bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-ima= ge"), options, >> "test", flags, false, false, &local_err); >> if (ret < 0) { >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 0b4335c..c8e8ba7 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, i= nt64_t total_size, >> goto out; >> } >> =20 >> - bdrv_close(bs); >> + bdrv_unref(bs); >> + bs =3D NULL; >> =20 >> /* >> * And now open the image and make it consistent first (i.e. inc= rease the >> @@ -1552,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, >> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local= _err); >> if (ret < 0) { >> error_propagate(errp, local_err); >> @@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename,= int64_t total_size, >> } >> } >> =20 >> - bdrv_close(bs); >> + bdrv_unref(bs); >> + 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, >> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKIN= G, >> drv, &local_err); >> if (error_is_set(&local_err)) { >> @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, i= nt64_t total_size, >> =20 >> ret =3D 0; >> out: >> - bdrv_unref(bs); >> + if (bs) { >> + bdrv_unref(bs); >> + } >> return ret; >> } >> =20 >> diff --git a/block/vmdk.c b/block/vmdk.c >> index e809e2e..52cd12a 100644 >> --- a/block/vmdk.c >> +++ b/block/vmdk.c >> @@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QE= MUOptionParameter *options, >> goto exit; >> } >> if (backing_file) { >> - BlockDriverState *bs =3D bdrv_new(""); >> - ret =3D bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, = NULL, errp); >> + BlockDriverState *bs =3D NULL; >> + ret =3D bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING,= NULL, errp); >> if (ret !=3D 0) { >> - bdrv_unref(bs); >> goto exit; >> } >> if (strcmp(bs->drv->format_name, "vmdk")) { >> diff --git a/block/vvfat.c b/block/vvfat.c >> index 664941c..ae7bc6f 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState = *s) >> goto err; >> } >> =20 >> - s->qcow =3D bdrv_new(""); >> - >> - ret =3D bdrv_open(s->qcow, s->qcow_filename, NULL, >> + s->qcow =3D NULL; >> + ret =3D bdrv_open(&s->qcow, s->qcow_filename, NULL, >> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qc= ow, >> &local_err); >> if (ret < 0) { >> qerror_report_err(local_err); >> error_free(local_err); >> - bdrv_unref(s->qcow); >> goto err; >> } >> =20 >> diff --git a/blockdev.c b/blockdev.c >> index 36ceece..6dd351c 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, &e= rror); >> + ret =3D bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &= error); >> =20 >> if (ret < 0) { >> error_setg(errp, "could not open disk image %s: %s", >> @@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTrans= actionState *common, >> qstring_from_str(snapshot_node_name)); >> } >> =20 >> - /* We will manually add the backing_hd field to the bs later */ >> - state->new_bs =3D bdrv_new(""); >> /* TODO Inherit bs->options or only take explicit options with a= n >> * extended QMP command? */ >> - ret =3D bdrv_open(state->new_bs, new_image_file, options, >> + assert(state->new_bs =3D=3D NULL); >> + ret =3D bdrv_open(&state->new_bs, new_image_file, 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) { >> error_propagate(errp, local_err); >> } >> @@ -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_err= ); >> + ret =3D bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_er= r); >> if (ret < 0) { >> error_propagate(errp, local_err); >> return; >> @@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const= char *target, >> return; >> } >> =20 >> - target_bs =3D bdrv_new(""); >> - ret =3D bdrv_open(target_bs, target, NULL, flags, drv, &local_err= ); >> + target_bs =3D NULL; >> + ret =3D bdrv_open(&target_bs, target, NULL, flags, drv, &local_er= r); >> if (ret < 0) { >> - bdrv_unref(target_bs); >> error_propagate(errp, local_err); >> return; >> } >> @@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, cons= t char *target, >> /* Mirroring takes care of copy-on-write using the source's back= ing >> * file. >> */ >> - target_bs =3D bdrv_new(""); >> - ret =3D bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACK= ING, drv, >> + target_bs =3D NULL; >> + ret =3D bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BAC= KING, drv, >> &local_err); >> if (ret < 0) { >> - bdrv_unref(target_bs); >> error_propagate(errp, local_err); >> return; >> } >> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c >> index 098f6c6..14e6d0b 100644 >> --- a/hw/block/xen_disk.c >> +++ b/hw/block/xen_disk.c >> @@ -813,7 +813,7 @@ 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, >> + if (bdrv_open(&blkdev->bs, >> blkdev->filename, NULL, qflags, drv, &loca= l_err) !=3D 0) >> { >> xen_be_printf(&blkdev->xendev, 0, "error: %s\n", >> diff --git a/include/block/block.h b/include/block/block.h >> index 963a61f..980869d 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -190,7 +190,7 @@ 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 *bs, const char *filename, QDict *opti= ons, >> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *op= tions, >> int flags, BlockDriver *drv, Error **errp); >> BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, >> BlockDriverState *bs, int flags)= ; >> diff --git a/qemu-img.c b/qemu-img.c >> index c989850..897aa56 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, flags, drv, &local_err); >> if (ret < 0) { >> error_report("Could not open '%s': %s", filename, >> error_get_pretty(local_err)); >> @@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char = *filename, >> } >> return bs; >> fail: >> - if (bs) { >> - bdrv_unref(bs); >> - } >> + bdrv_unref(bs); >> return NULL; >> } >> =20 >> @@ -2314,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, BDRV_O= _FLAGS, >> old_backing_drv, &local_err); >> if (ret) { >> error_report("Could not open old backing file '%s': %s", >> @@ -2324,7 +2322,7 @@ 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, BDRV= _O_FLAGS, >> + ret =3D bdrv_open(&bs_new_backing, out_baseimg, NULL, BDR= V_O_FLAGS, >> new_backing_drv, &local_err); >> if (ret) { >> error_report("Could not open new backing file '%s': = %s", >> diff --git a/qemu-io.c b/qemu-io.c >> index 7f459d8..8da8f6e 100644 >> --- a/qemu-io.c >> +++ b/qemu-io.c >> @@ -68,7 +68,7 @@ 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, opts, flags, NULL, &local_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 136e8c9..0cf123c 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, flags, drv, &local_err); >> if (ret < 0) { >> errno =3D -ret; >> err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optin= d], >> --=20 >> 1.8.5.4 >> >>