From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55453) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFkyD-0007Lm-DY for qemu-devel@nongnu.org; Tue, 18 Feb 2014 08:44:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFky8-0007an-DE for qemu-devel@nongnu.org; Tue, 18 Feb 2014 08:44:21 -0500 Received: from lnantes-156-75-100-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:33598 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFky8-0007ag-3U for qemu-devel@nongnu.org; Tue, 18 Feb 2014 08:44:16 -0500 Date: Tue, 18 Feb 2014 14:44:14 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140218134414.GE18400@irqsave.net> References: <1392435024-26694-1-git-send-email-mreitz@redhat.com> <1392435024-26694-8-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1392435024-26694-8-git-send-email-mreitz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 7/8] block: Reuse success path from bdrv_open() 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 The Saturday 15 Feb 2014 =E0 04:30:23 (+0100), Max Reitz wrote : > The fail and success paths of bdrv_file_open() may be further shortened > by reusing code already existent in bdrv_open(). This includes > bdrv_file_open() not taking the reference to options which allows the > removal of QDECREF(options) in that function. >=20 > Signed-off-by: Max Reitz > --- > block.c | 63 +++++++++++++++++++++++++++++----------------------------= ------ > 1 file changed, 29 insertions(+), 34 deletions(-) >=20 > diff --git a/block.c b/block.c > index c11d4d0..7e2dc81 100644 > --- a/block.c > +++ b/block.c > @@ -948,13 +948,15 @@ free_and_fail: > /* > * Opens a file using a protocol (file, host_device, nbd, ...) > * > - * options is a QDict of options to pass to the block drivers, or NULL= for an > - * 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_file_open= . > + * options is an indirect pointer to a QDict of options to pass to the= block > + * drivers, or pointer to NULL for an empty set of options. If this fu= nction > + * takes ownership of the QDict reference, it will set *options to NUL= L; > + * otherwise, it will contain unused/unrecognized options after this f= unction > + * returns. Then, the caller is responsible for freeing it. If it inte= nds to > + * reuse the QDict, QINCREF() should be called beforehand. > */ > static int bdrv_file_open(BlockDriverState *bs, const char *filename, > - QDict *options, int flags, Error **errp) > + QDict **options, int flags, Error **errp) > { > BlockDriver *drv; > const char *drvname; > @@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, con= st char *filename, > =20 > /* Fetch the file name from the options QDict if necessary */ > if (!filename) { > - filename =3D qdict_get_try_str(options, "filename"); > - } else if (filename && !qdict_haskey(options, "filename")) { > - qdict_put(options, "filename", qstring_from_str(filename)); > + filename =3D qdict_get_try_str(*options, "filename"); > + } else if (filename && !qdict_haskey(*options, "filename")) { > + qdict_put(*options, "filename", qstring_from_str(filename)); > allow_protocol_prefix =3D true; > } else { > error_setg(errp, "Can't specify 'file' and 'filename' options = at the " > @@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, c= onst char *filename, > } > =20 > /* Find the right block driver */ > - drvname =3D qdict_get_try_str(options, "driver"); > + drvname =3D qdict_get_try_str(*options, "driver"); > if (drvname) { > drv =3D bdrv_find_format(drvname); > if (!drv) { > error_setg(errp, "Unknown driver '%s'", drvname); > } > - qdict_del(options, "driver"); > + qdict_del(*options, "driver"); > } else if (filename) { > drv =3D bdrv_find_protocol(filename, allow_protocol_prefix); > if (!drv) { > @@ -1001,41 +1003,30 @@ static int bdrv_file_open(BlockDriverState *bs,= const char *filename, > =20 > /* Parse the filename and open it */ > if (drv->bdrv_parse_filename && filename) { > - drv->bdrv_parse_filename(filename, options, &local_err); > + drv->bdrv_parse_filename(filename, *options, &local_err); > if (error_is_set(&local_err)) { > error_propagate(errp, local_err); > ret =3D -EINVAL; > goto fail; > } > - qdict_del(options, "filename"); > + qdict_del(*options, "filename"); > } > =20 > if (!drv->bdrv_file_open) { > - ret =3D bdrv_open(&bs, filename, NULL, options, flags, drv, &l= ocal_err); > - options =3D NULL; > + 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, &local= _err); > + ret =3D bdrv_open_common(bs, NULL, *options, flags, drv, &loca= l_err); > } > if (ret < 0) { > error_propagate(errp, local_err); > goto fail; > } > =20 > - /* Check if any unknown options were used */ > - if (options && (qdict_size(options) !=3D 0)) { > - const QDictEntry *entry =3D qdict_first(options); > - error_setg(errp, "Block protocol '%s' doesn't support the opti= on '%s'", > - drv->format_name, entry->key); > - ret =3D -EINVAL; > - goto fail; > - } > - QDECREF(options); > - > bs->growable =3D 1; > return 0; > =20 > fail: > - QDECREF(options); > return ret; > } > =20 > @@ -1247,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const cha= r *filename, > =20 > if (flags & BDRV_O_PROTOCOL) { > assert(!drv); > - ret =3D bdrv_file_open(bs, filename, options, flags & ~BDRV_O_= PROTOCOL, > + ret =3D bdrv_file_open(bs, filename, &options, flags & ~BDRV_O= _PROTOCOL, > &local_err); > - options =3D NULL; > if (!ret) { > - *pbs =3D bs; > - return 0; > + goto done; > } else if (bs->drv) { > goto close_and_fail; > } else { > @@ -1389,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const cha= r *filename, > } > } > =20 > +done: > /* Check if any unknown options were used */ > - if (qdict_size(options) !=3D 0) { > + if (options && (qdict_size(options) !=3D 0)) { > const QDictEntry *entry =3D qdict_first(options); > - error_setg(errp, "Block format '%s' used by device '%s' doesn'= t " > - "support the option '%s'", drv->format_name, bs->de= vice_name, > - entry->key); > + if (flags & BDRV_O_PROTOCOL) { > + error_setg(errp, "Block protocol '%s' doesn't support the = option " > + "'%s'", drv->format_name, entry->key); > + } else { > + error_setg(errp, "Block format '%s' used by device '%s' do= esn't " > + "support the option '%s'", drv->format_name, > + bs->device_name, entry->key); > + } > =20 > ret =3D -EINVAL; > goto close_and_fail; > --=20 > 1.8.5.4 >=20 Reviewed-by: Benoit Canet