From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33431) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W7rUQ-0005Xy-1h for qemu-devel@nongnu.org; Mon, 27 Jan 2014 14:05:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W7rUJ-0000XR-UU for qemu-devel@nongnu.org; Mon, 27 Jan 2014 14:04:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51583) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W7rUJ-0000Wm-NQ for qemu-devel@nongnu.org; Mon, 27 Jan 2014 14:04:51 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s0RJ4nO1002326 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 27 Jan 2014 14:04:49 -0500 Message-ID: <52E6AE4A.4060304@redhat.com> Date: Mon, 27 Jan 2014 20:06:50 +0100 From: Max Reitz MIME-Version: 1.0 References: <1390762963-25538-1-git-send-email-mreitz@redhat.com> <1390762963-25538-10-git-send-email-mreitz@redhat.com> <20140127174414.GB28138@localhost.localdomain> In-Reply-To: <20140127174414.GB28138@localhost.localdomain> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 09/10] block: Reuse success path from bdrv_open() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 27.01.2014 18:44, Jeff Cody wrote: > On Sun, Jan 26, 2014 at 08:02:42PM +0100, Max Reitz wrote: >> The fail and success paths of bdrv_file_open() may be further shortene= d >> 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. >> >> Signed-off-by: Max Reitz >> --- >> block.c | 33 ++++++++++++--------------------- >> 1 file changed, 12 insertions(+), 21 deletions(-) >> >> diff --git a/block.c b/block.c >> index f847c4b..b1bae23 100644 >> --- a/block.c >> +++ b/block.c >> @@ -943,9 +943,7 @@ 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 NU= LL for an >> - * empty set of options. The reference to the QDict belongs to the bl= ock layer >> - * after the call (even on failure), so if the caller intends to reus= e the >> - * dictionary, it needs to use QINCREF() before calling bdrv_file_ope= n. >> + * empty set of options. >> */ >> static int bdrv_file_open(BlockDriverState *bs, const char *filename= , >> QDict *options, int flags, Error **errp) >> @@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, = const char *filename, >> } >> =20 >> if (!drv->bdrv_file_open) { >> + QINCREF(options); >> 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); >> } >> @@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs= , const char *filename, >> 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 opt= ion '%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 >> @@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char= *filename, >> assert(!drv); >> ret =3D bdrv_file_open(bs, filename, options, flags & ~BDRV_= O_PROTOCOL, >> &local_err); >> - options =3D NULL; >> if (ret) { >> if (bs->drv) { >> goto close_and_fail; >> @@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char= *filename, >> goto fail; >> } >> } >> - *pbs =3D bs; >> - return 0; >> + goto done; >> } >> =20 >> /* For snapshot=3Don, create a temporary qcow2 overlay */ >> @@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const ch= ar *filename, >> } >> } >> =20 >> +done: >> /* Check if any unknown options were used */ >> if (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->d= evice_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); > Tests 071 and 072 segfault, and gdb shows that it occurs here. More > investigation shows that entry is NULL, although qdict_size() is > returning 1. Hm, and I thought I'd run the tests, but obviously I did not for this=20 final version=85 The problem is probably the QINCREF(). Hm, I really want= =20 to reuse this test; the only way I see then is to make the options=20 parameter of bdrv_file_open() an indirect pointer so it can be set to=20 NULL after bdrv_open() (in bdrv_file_open()). That would not be very=20 pretty, but it would work and there wouldn't be two tests whether all=20 options could be handled. Thanks for catching this, Max >> + } else { >> + error_setg(errp, "Block format '%s' used by device '%s' d= oesn'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.3 >> >>