From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCqk7-0004fJ-Vq for qemu-devel@nongnu.org; Mon, 10 Feb 2014 08:17:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCqk1-0004PV-UU for qemu-devel@nongnu.org; Mon, 10 Feb 2014 08:17:47 -0500 Received: from paradis.irqsave.net ([62.212.105.220]:49558) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCqk1-0004PD-2c for qemu-devel@nongnu.org; Mon, 10 Feb 2014 08:17:41 -0500 Date: Mon, 10 Feb 2014 14:17:37 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140210131737.GE3088@irqsave.net> References: <1391881159-13585-1-git-send-email-mreitz@redhat.com> <1391881159-13585-2-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1391881159-13585-2-git-send-email-mreitz@redhat.com> 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: Max Reitz Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Stefan Hajnoczi 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). >=20 > 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(-) >=20 > 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_er= r); > + ret =3D bdrv_open(&bs, filename, options, flags, drv, &local_e= rr); > options =3D NULL; > } else { > ret =3D bdrv_open_common(bs, NULL, options, flags, drv, &local= _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 *bs= , 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, optio= ns, > 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 wit= h a > + * pointer to it stored there. If it is not NULL, the referenced BDS w= ill > + * 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, const= char *filename, > /* If a filename is given and the block driver should be detec= ted > automatically (instead of using none), use bdrv_open() in o= rder to do > that auto-detection. */ > - BlockDriverState *bs; > - > if (reference) { > error_setg(errp, "Cannot reference an existing block devic= e while " > "giving a filename"); > @@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, cons= t char *filename, > goto done; > } > =20 > - bs =3D bdrv_new(""); > - ret =3D bdrv_open(bs, filename, image_options, flags, NULL, er= rp); > - if (ret < 0) { > - bdrv_unref(bs); > - } else { > - *pbs =3D bs; > - } > + ret =3D bdrv_open(pbs, filename, image_options, flags, NULL, e= rrp); > } else { > ret =3D bdrv_file_open(pbs, filename, reference, image_options= , flags, > errp); > @@ -1223,22 +1217,32 @@ done: > * empty set of options. The reference to the QDict belongs to the blo= ck layer > * after the call (even on failure), so if the caller intends to reuse= 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 sto= red there. > + * If it is not NULL, the referenced BDS will be reused. > */ > -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *optio= ns, > +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *opt= ions, > int flags, BlockDriver *drv, Error **errp) > { > int ret; > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows.= */ > 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. > 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 *f= ilename, 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), t= rue, true, > &local_err); is_temporary can already be set to one the goto after the bdrv_open_image= should be unlink_and_fail yet I don't know if the fix belong to this patch. > @@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *f= ilename, 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 functio= n and > + needs to be freed now. Otherwise, it does not need to be cl= osed, > + 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_BA= CKING); > =20 > - bs =3D bdrv_new(""); > - > - ret =3D bdrv_open(bs, backing_file->value.s, NULL, back_fl= ags, > + bs =3D NULL; > + ret =3D bdrv_open(&bs, backing_file->value.s, NULL, back_f= lags, > 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, QDic= t *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, QDi= ct *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"), op= tions, > "raw", flags, true, false, &local_err); > if (ret < 0) { > @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDi= ct *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-image= "), 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, in= t64_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. incre= ase the > @@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, in= t64_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_e= rr); > 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 ret= urning */ > - ret =3D bdrv_open(bs, filename, NULL, > + ret =3D bdrv_open(&bs, filename, NULL, > BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, > drv, &local_err); > if (error_is_set(&local_err)) { > @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, in= t64_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, QEM= UOptionParameter *options, > goto exit; > } > if (backing_file) { > - BlockDriverState *bs =3D bdrv_new(""); > - ret =3D bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, N= ULL, 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_qcow= , > &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, Q= Dict *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, &er= ror); > + ret =3D bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &e= rror); > =20 > if (ret < 0) { > error_setg(errp, "could not open disk image %s: %s", > @@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransa= ctionState *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 an > * 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(BlockDriverSt= ate *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_err= ); > 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_err= ); > if (ret < 0) { > - bdrv_unref(target_bs); > error_propagate(errp, local_err); > return; > } > @@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const= char *target, > /* Mirroring takes care of copy-on-write using the source's backin= g > * file. > */ > - target_bs =3D bdrv_new(""); > - ret =3D bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKI= NG, drv, > + target_bs =3D NULL; > + ret =3D bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACK= ING, 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, &local_= 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 c= har *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 *bs, const char *filename, QDict *optio= ns, > +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *opt= ions, > 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_nam= e)); > - ret =3D bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_F= LAGS, > + 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, BDRV= _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 growab= le, 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", progname= , 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[optind]= , > --=20 > 1.8.5.4 >=20 >=20