From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz6Cw-0004RY-VP for qemu-devel@nongnu.org; Mon, 23 Jun 2014 11:31:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wz6Co-0001RC-Ku for qemu-devel@nongnu.org; Mon, 23 Jun 2014 11:30:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38418) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz6Co-0001Qv-DG for qemu-devel@nongnu.org; Mon, 23 Jun 2014 11:30:50 -0400 Date: Mon, 23 Jun 2014 17:30:45 +0200 From: Kevin Wolf Message-ID: <20140623153045.GH10973@noname.redhat.com> References: <1402495503-4722-1-git-send-email-kwolf@redhat.com> <1402495503-4722-2-git-send-email-kwolf@redhat.com> <20140612120839.GA24528@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140612120839.GA24528@irqsave.net> 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: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com Am 12.06.2014 um 14:08 hat Beno=EEt Canet geschrieben: > 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 t= he > > 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 t= he block > > - * drivers, or pointer to NULL for an empty set of options. If this = function > > - * takes ownership of the QDict reference, it will set *options to N= ULL; > > - * otherwise, it will contain unused/unrecognized options after this= function > > - * returns. Then, the caller is responsible for freeing it. If it in= tends to > > - * reuse the QDict, QINCREF() should be called beforehand. > > + * Fills in default options for opening images and converts the lega= cy > > + * 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' option= s at the " > > - "same time"); > > - ret =3D -EINVAL; > > - goto fail; > > + if (filename) { > > + if (filename && !qdict_haskey(*options, "filename")) { >=20 > The inner filename testing is redundant. Will fix. > > + qdict_put(*options, "filename", qstring_from_str(filenam= e)); > > + parse_filename =3D true; > > + } else { > > + error_setg(errp, "Can't specify 'file' and 'filename' op= tions 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"); >=20 > Isn't it "Must specify either driver or _filename_ ? This is only code motion, but changing it in a separate patch would make sense, I guess. > > + 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 t= he block > > + * drivers, or pointer to NULL for an empty set of options. If this = function > > + * takes ownership of the QDict reference, it will set *options to N= ULL; > > + * otherwise, it will contain unused/unrecognized options after this= function > > + * returns. Then, the caller is responsible for freeing it. If it in= tends 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 ? Okay. Rather pointless and the function will be gone at the end of the series, but more consistent indeed. > > + } > > + > > + 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) { >=20 > Don't we need: > qdict_del(*options, "filename"); > Here so bdrv_open don't see an extra filename option ? I think it doesn't matter because bdrv_open() would put it back again. Do you have an example where it makes a difference? > > ret =3D bdrv_open(&bs, filename, NULL, *options, flags, drv,= &local_err); > > *options =3D NULL; Kevin