From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv3oL-0002mV-6i for qemu-devel@nongnu.org; Thu, 12 Jun 2014 08:09:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wv3oD-0001b8-HS for qemu-devel@nongnu.org; Thu, 12 Jun 2014 08:08:53 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:46644 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wv3oD-0001aE-7V for qemu-devel@nongnu.org; Thu, 12 Jun 2014 08:08:45 -0400 Date: Thu, 12 Jun 2014 14:08:40 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140612120839.GA24528@irqsave.net> References: <1402495503-4722-1-git-send-email-kwolf@redhat.com> <1402495503-4722-2-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1402495503-4722-2-git-send-email-kwolf@redhat.com> Content-Transfer-Encoding: quoted-printable 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 Cc: qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com The Wednesday 11 Jun 2014 =E0 16:04:55 (+0200), 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 > diff --git a/block.c b/block.c > index 17f763d..c5707e8 100644 > --- a/block.c > +++ b/block.c > @@ -1007,77 +1007,107 @@ free_and_fail: > } > =20 > /* > - * Opens a file using a protocol (file, host_device, nbd, ...) > - * > - * 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. > + * 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) > { > - 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 inner filename testing 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); > + 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"); Isn't it "Must specify either driver or _filename_ ? > + return -EINVAL; > } > - } else { > - error_setg(errp, "Must specify either driver or file"); > - drv =3D NULL; > } > =20 > + drv =3D bdrv_find_format(drvname); > if (!drv) { > - /* errp has been set already */ > - ret =3D -ENOENT; > - goto fail; > + error_setg(errp, "Unknown driver '%s'", drvname); > + return -ENOENT; > } > =20 > - /* Parse the filename and open it */ > + /* Driver-specific filename parsing */ > if (drv->bdrv_parse_filename && parse_filename) { > drv->bdrv_parse_filename(filename, *options, &local_err); > if (local_err) { > error_propagate(errp, local_err); > - ret =3D -EINVAL; > - goto fail; > + return -EINVAL; > } > =20 > if (!drv->bdrv_needs_filename) { > qdict_del(*options, "filename"); > - } else { > - filename =3D qdict_get_str(*options, "filename"); > } > } > =20 > + return 0; > +} > + > +/* > + * Opens a file using a protocol (file, host_device, nbd, ...) > + * > + * 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) > +{ > + BlockDriver *drv; > + const char *drvname; > + Error *local_err =3D NULL; > + int ret; > + > + ret =3D bdrv_fill_options(options, filename, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return ret; Why not goto fail for consistency with the rest of the function ? > + } > + > + filename =3D qdict_get_try_str(*options, "filename"); > + drvname =3D qdict_get_str(*options, "driver"); > + > + drv =3D bdrv_find_format(drvname); > + if (!drv) { > + error_setg(errp, "Unknown driver '%s'", drvname); > + ret =3D -ENOENT; > + goto fail; > + } > + qdict_del(*options, "driver"); > + > + /* Open the file */ > if (!drv->bdrv_file_open) { Don't we need: qdict_del(*options, "filename"); Here so bdrv_open don't see an extra filename option ? > ret =3D bdrv_open(&bs, filename, NULL, *options, flags, drv, &= local_err); > *options =3D NULL; > --=20 > 1.8.3.1 >=20 >=20