From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50944) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wunys-0005pv-63 for qemu-devel@nongnu.org; Wed, 11 Jun 2014 15:14:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wunyn-0006HJ-0i for qemu-devel@nongnu.org; Wed, 11 Jun 2014 15:14:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51384) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wunym-0006HD-OK for qemu-devel@nongnu.org; Wed, 11 Jun 2014 15:14:36 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5BJEZkE023789 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 11 Jun 2014 15:14:36 -0400 Message-ID: <5398AA9A.6010409@redhat.com> Date: Wed, 11 Jun 2014 13:14:34 -0600 From: Eric Blake MIME-Version: 1.0 References: <1402495503-4722-1-git-send-email-kwolf@redhat.com> <1402495503-4722-2-git-send-email-kwolf@redhat.com> In-Reply-To: <1402495503-4722-2-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UD2Aw4SA6H7rmC6k3uu7rJQkFCn6SoSiC" Subject: Re: [Qemu-devel] [PATCH 1/9] block: Create bdrv_fill_options() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-devel@nongnu.org Cc: armbru@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --UD2Aw4SA6H7rmC6k3uu7rJQkFCn6SoSiC Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/11/2014 08:04 AM, Kevin Wolf wrote: > The idea of bdrv_fill_options() is to convert every parameter for > opening images, in particular the filename and flags, to entries in the= > options QDict. >=20 > This patch starts with moving the filename parsing and driver probing > part from bdrv_file_open() to the new function. >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 116 ++++++++++++++++++++++++++++++++++++++++----------------= -------- > 1 file changed, 73 insertions(+), 43 deletions(-) >=20 > + * Fills in default options for opening images and converts the legacy= > + * filename/flags pair to option QDict entries. > */ > -static int bdrv_file_open(BlockDriverState *bs, const char *filename, > - QDict **options, int flags, Error **errp) > +static int bdrv_fill_options(QDict **options, const char *filename, > + Error **errp) The comment mentions filename/flags, but I only see filename as a parameter. Is this something changed later in the series? > { > - BlockDriver *drv; > const char *drvname; > bool parse_filename =3D false; > Error *local_err =3D NULL; > - int ret; > + BlockDriver *drv; > =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)); > - parse_filename =3D true; > - } else { > - error_setg(errp, "Can't specify 'file' and 'filename' options = at the " > - "same time"); > - ret =3D -EINVAL; > - goto fail; > + if (filename) { > + if (filename && !qdict_haskey(*options, "filename")) { The 'filename &&' is redundant. > + qdict_put(*options, "filename", qstring_from_str(filename)= ); > + parse_filename =3D true; > + } else { > + error_setg(errp, "Can't specify 'file' and 'filename' opti= ons at " > + "the same time"); > + return -EINVAL; > + } > } > =20 > /* Find the right block driver */ > + filename =3D qdict_get_try_str(*options, "filename"); > 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"); > - } else if (filename) { > - drv =3D bdrv_find_protocol(filename, parse_filename); > - if (!drv) { > - error_setg(errp, "Unknown protocol"); > + > + if (!drvname) { > + if (filename) { > + drv =3D bdrv_find_protocol(filename, parse_filename); This assigns drv... > + if (!drv) { > + error_setg(errp, "Unknown protocol"); > + return -EINVAL; > + } > + > + drvname =3D drv->format_name; > + qdict_put(*options, "driver", qstring_from_str(drvname)); > + } else { > + error_setg(errp, "Must specify either driver or file"); > + return -EINVAL; > } > - } else { > - error_setg(errp, "Must specify either driver or file"); > - drv =3D NULL; > } > =20 > + drv =3D bdrv_find_format(drvname); =2E..and this does it again. Should this second assignment be inside an else{}? > +static int bdrv_file_open(BlockDriverState *bs, const char *filename, > + QDict **options, int flags, Error **errp) > + > + drv =3D bdrv_find_format(drvname); > + if (!drv) { > + error_setg(errp, "Unknown driver '%s'", drvname); > + ret =3D -ENOENT; > + goto fail; > + } Isn't this error check redundant with the one already done in bdrv_fill_options()? You could probably just use assert(drv) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --UD2Aw4SA6H7rmC6k3uu7rJQkFCn6SoSiC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTmKqaAAoJEKeha0olJ0Nq8KcH/RNfbQ65k+3xyAokf3ClnHw0 peEVCo1Yk0cUG+uE4bYA+JYB9nEmLrodtiun5WNAdm4gFMFi68NJ0+W4jRJMbgDk SJwGc7f12dPaD9pBszW21QD/fOpAP8HV5KZQDqLyqf9FUkC5LDqPfnTwVhlwjsBk Ku1T8uPWBRU+X6PER6CTQcdrAmFR5OaEK9lWY3eW2i5SQNmGlrmxt2p0ODGRMtlh HFO5rbss5b9zUqt1gHNro/Tk9qQq2C88skQ6JSq99F/VBG8yw/uPJx3E7l2ZQP8n rpE2v20GTRtvt1fFnSlT80J+XXbSzjDzMR62BbrAEIqmAQZ8v8hGy1FJzWz1snw= =r75j -----END PGP SIGNATURE----- --UD2Aw4SA6H7rmC6k3uu7rJQkFCn6SoSiC--